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)