W
W
WebDev2020-07-22 15:16:23
PHP
WebDev, 2020-07-22 15:16:23

Is this the right way to organize the code?

Hello. I needed to develop an API to get data from another API.
The task itself is elementary, but I am tormented by the question of how to do it more correctly.
Here's how I did it:

1) Created a Services directory in app and added it to autoload in composer. Created a Partner class that can send and receive songs.

//App\Services
class Partner
{
    /**
     * Returns an object of song of certain singer.
     *
     * @param  string  $singer
     * @param  string  $song_name
     * @return array
     */
    public function getSong(string $singer, string $song_name) : array
    {
        try {
            $json = file_get_contents('partner-domain.com/api?singer=' . $singer);
        } catch (\Exception $e) {
             //throw new Exception or return object with status?
        }

        $songs = json_decode($json, true);
    
        //Filter songs by given name
        $result = array_filter($songs, function($song) use ($song_name) {
            return mb_strtolower($song['name'] === mb_strtolower($song_name));
        });

         //We need to return only 1 song
        return $result[0] ?? [];
    }

    /**
     * Send a song to API.
     *
     * @param  string  $singer
     * @param  object  $song
     * @return void
     */
    public function sendSong(string $singer, object $song) : void
    {
        try {
            $curl = curl_init('partner-domain.com/api/send');
            /*curl body, postfields etc...*/
            curl_exec($curl);
            curl_close($curl);
        } catch (\Exception $e) {
            //throw new Exception or return object with status?
        }
    }
}


2) Created a route and a method in the controller:
//App\Http\Controllers

class ApiController()
{
    private $partnerService;

    public function __constructor()
    {
        $this->partnerService = new App\Services\Partner();
    }

    /**
     * Returns an object of song of certain singer.
     *
     * @param  string  $singer
     * @param  string  $song_name
     * @return string
     */
    public function getSong(string $singer, string $song_name) : string
    {
         $song = $this->partnerService->getSong($singer, $song_name);

         return response()->json($song);
    }

    /**
     * Send a song to API.
     *
     * @param  string  $singer
     * @param  object $song
     * @return string
     */
    public function sendSong(string $singer, object $song) : void
    {
         $this->partnerService->sendSong($singer, $song);
    }
}


I have a few questions about this code:
1) How to properly handle errors with access to a partner service? In the Partner class, in the getSong method, it is more correct to throw an exception and catch it in the controller (but for this you will have to wrap all method calls of the Partner class in try-catch) or simply return an object with the 'status' key, where to write 'ok' in case of success and ' error' in case something went wrong, something like this:
//App\Services\Partner
public function getSong(string $singer, string $song_name) : array
    {
        try {
            $json = file_get_contents('partner-domain.com/api?singer=' . $singer);
        } catch (\Exception $e) {
            return [
                'status' => 'error',
                'message' => $e->getMessage();
            ];
        }

        $songs = json_decode($json, true);

        //Filter songs by given name
        $result = array_filter($songs, function($song) use ($song_name) {
            return mb_strtolower($song['name'] === mb_strtolower($song_name));
        });

        //We need to return only 1 song
        if (isset($result[0])) {
             return [
                'status' => 'ok',
                'data' => $result[0];
            ];
        } else {
             return [
                'status' => 'error',
                'message' => 'not found';
            ];
        }
}


2) Is it correct to connect your classes in the controller (through the constructor) in this way?
3) The question is similar to the first one, but concerns the sendSong method. What if an error occurs there? What is the correct way to return it from the class to the controller and from the controller to the user?
4) In php, you can't specify two return types, right? And you can not specify mixed. Does this mean that if there is no such song in the getSong method, then I cannot return, for example, null instead of an array? You have to return an empty array. It is right?

I will be very grateful for detailed answers.
Thank you.

Answer the question

In order to leave comments, you need to log in

2 answer(s)
M
Maxim, 2020-07-22
@kirill-93

  1. Better try/catch. If used like this, then your response form may be different depending on the service. The service throws an error, and in the controller or intermediate layer it is caught and formatted. At the same time, controllers can be different and each has its own response format: Web, API, Console
  2. It is better to pass to the constructor itself and use DI.
  3. In the method, just throw exception A in the try/catch controller.throw new Exeption('Ошибка!');
  4. Not yet. Will only be in php8. The get method must return an error or data. The find method must return null or data.

D
dmitriy, 2020-07-22
@dmitriylanets

1) at least one PartnerException must come from the Partner class,
all exceptions in the Partner class will be "digested into one" PartnerException
in your case, you can add another heir and PartnerNotFoundException
logic becomes more concise:

//App\Services\Partner
public function getSong(string $singer, string $song_name) : array
    {
        try {
            $json = file_get_contents('partner-domain.com/api?singer=' . $singer);
        } catch (\Exception $e) {
            throw new PartnerException($e->getMessage());
        }

        $songs = json_decode($json, true);

        //Filter songs by given name
        $result = array_filter($songs, function($song) use ($song_name) {
            return mb_strtolower($song['name'] === mb_strtolower($song_name));
        });

        //We need to return only 1 song
        if (!isset($result[0])) {
              throw new PartnerNotFoundException(sprintf(
                   'parnter not found by %s and %s', $singer, $song_name
            ));
        } 

       return [
                'status' => 'ok',
                'data' => $result[0];
      ];
}

- in the try catch controller if you need to catch
public function getSong(string $singer, string $song_name) : string
    {
         try{
             $song = $this->partnerService->getSong($singer, $song_name);
         }
        catch(PartnerNotFoundException $ex){

            //п.с не помню как в лаевел но смысл поняли
             return response()->status(404)->send();
        }

         return response()->json($song);
    }

2) through the constructor
3) similarly to p1
4) the method is called getSong to get the song, which means we drive all alternative options into exceptions, there is no song - an exception, a service error, an exception

Didn't find what you were looking for?

Ask your question

Ask a Question

731 491 924 answers to any question