Laravel controller Single Responsibility principle

Question

I have a method in Laravel controller. How can I improve this code? How can I implement single responsibilty here or maybe there are some other tips I can use in this code? I know that controller methods should be kept as small as possible,but I don't know if it's better idea to split this code in separate methods.

public function displayData(CityRequest $request)
    {
        if (isset($request->validator) && $request->validator->fails()) {
            return redirect()->back()->withErrors($request->validator->messages());
        }
        else {
            $cityName = $request->city;
            $cityName = str_replace(' ','-',$cityName);
            $cityName = iconv('UTF-8', 'ISO-8859-1//TRANSLIT//IGNORE', $cityName);
            $response = $this->guzzle->request('GET', asset("/api/products/recommended/" . $cityName));
            $response_body = json_decode($response->getBody());
            if(isset($response_body->error))
            {
                return redirect()->back()->withErrors(['city'=>$response_body->error]);
            }
            $city = $response_body->city;
            $condition = $response_body->current_weather;
            $products = $response_body->recommended_products;
            return back()->with(['city' => $city, 'condition' => $condition, 'products' => $products]);
        }
    }

Answer 1

Here are some simple tips on how you can improve your code. There are still more you can do to improve it, but is a good start and it helps you get closer to the single responsibility principle.

• Move the API request to a service (App\Services\ProductsApi\Client).
• Move the formatting of city to a helper (App\Helpers\Format).
• Consider moving the formatting of city to CityRequest so the controller doesn't have to do it.
• No need for new variables, use the properties returned by $response instead.
• No need for else-statement, since the if-statement has return.

Your controller:

use App\Services\ProductsApi\Client;

public function __construct(Client $client)
{
    $this->client = $client;
}

public function displayData(CityRequest $request)
{
    if (isset($request->validator) && $request->validator->fails()) {
        return redirect()->back()->withErrors($request->validator->messages());
    }

    $response = $this->client->getRecommended(
        App\Helpers\Format::slugify($request->city)
    );

    if ($response->error) {
        return redirect()->back()->withErrors([
            'city' => $response->error
        ]);
    }

    return back()->with([
        'city' => $response->city ?? null, 
        'condition' => $response->current_weather ?? null,  
        'products' => $response->recommended_products ?? null, 
    ]);
}

Helper:

public static function slugify($value)
{
    return Illuminate\Support\Str::slug(
        iconv('UTF-8', 'ISO-8859-1//TRANSLIT//IGNORE', $value),
        '-'
    );
}

(None of the code above is tested)

Answer 2

First of all you should extract the else body in a private methode. Don't forget to return the result of this methode or else the User won't be redirected:

else{
  return $this->getProducts()
}

Then you could split this methode even more and extract the $cityName valdidation. Also, you usually don't do API-requests in a Controller, but it depends on your project if you should extract the request in a seperate Model.

Overall, I've seen much more bloated controllers, even in professional environments.

Answer 3

A typical controller does stuff like:

• Validate the incoming request
• Retrieve values from that request
• Serve a response

In your case the controller also does:

• Parsing of the request parameters
• Send API calls to an external endpoint
• Handle decoding and errors in external API calls

The last 3 can be split into a separate service (or even 3 separate services)