How to Write a Good Controller? One of the Most Common Mistakes Using the Example of Laravel

Everyone learns from mistakes. In this article I will point out a cardinal one, which leads to a situation where the controller contains several hundred lines of code and becomes unreadable and very difficult to maintain. However, I will present a few ways to solve it.

While implementing PHP development services at Droptica, I encountered this error many times, and even made it myself at the beginning. What am I talking about? About putting the entire logic in controller methods without dividing individual activities into separate components in the application.

To avoid this, follow two main rules:

1. You can have an unlimited number of controllers. It is better to have many that contain few methods than one controller that contains many methods.
2. The controller is only responsible for the overall logic. Following the general-to-particular principle, there should be a general part in the controller. The particulars, on the other hand should be included in other application components. The less code in the controller, the better.

Let us take a look at the example below:

class CustomersController extends Controller
{
    public function getActiveCustomers()
    {
        return Customer::where('active', 1)->get();
    }

    public function store()
    {
        $data = $this->validateRequest();
        $customer = Customer::create($data);

        // Send notification email to admin.
        Mail::to('[email protected]')->send(new NewCustomerMail($customer));

        // Send welcome mail to customer
        Mail::to($customer->email)->send(new WelcomeMail($customer));
    }

    public function getQueryResults()
    {
        $customers = Customer::query();

        if (request()->has('name')) {
            $customers->where('name', request('name'));
        }

        if (request()->has('active')) {
            $customers->where('active', request('active'));
        }

        if (request()->has('sort')) {
            $customers->orderBy('name', request('sort'));
        }

        $customers->get();

        return $customers;
    }

    private function validateRequest()
    {
        // Request validation code goes here…
    }
}

At first glance it may seem that everything is as it should be. The controller contains three methods. The first one of these returns the active clients, the second one creates a new client and sends e-mail notifications, while the third one is responsible for filtering the search results. So why is this example presented as being incorrect?

CRUD

Let us start with the first example. According to Laravel's documentation, the controller should contain the methods index, create, store, show, edit, update or/and delete. Therefore, the method should be renamed to one of the above. But for that to make sense, you need to follow the first rule. So, let us create a completely new controller – ActiveCustomersController, and move the getActiveCustomers method there, while changing its name to index.

class ActiveCustomersController extends Controller
{
    public function index()
    {
        return Customer::where('active', 1)->get();
    }
}

Remember that if your controller contains only one method, in Laravel you can rename that method to __invoke().

class ActiveCustomersController extends Controller
{
    public function __invoke()
    {
        return Customer::where('active', 1)->get();
    }
}

This is not the controller's task

Now let us take a look at the next method: store(). Its name is correct and so is the name of the controller. So where is the error hiding?

In this example, after creating a new client, two e-mail messages are being sent (to the client and to the administrator). It is not much, but let us try to imagine what will happen if in the future you will want to add to this a Slack notification and send an SMS (e.g. with a verification code). The amount of code will increase significantly, breaking the second rule. Therefore, we should look for another place in our application that will be responsible for sending notifications. Events work great for this purpose.

So, let us start by launching the event that will replace the code responsible for sending notifications:

public function store()
{
    $data = $this->validateRequest();
    $customer = Customer::create($data);

    event(new NewCustomerHasRegisteredEvent($customer));
}

Then create this event by typing in the console:

php artisan make:event NewCustomerHasRegisteredEvent

This command will create a new class – NewCustomerHasRegisteredEvent in the app/Events directory. Because in the controller you transfer the created client using a parameter, you should accept this data in the constructor in the event class.

The whole should look like this:

class NewCustomerHasRegisteredEvent
{
    use Dispatchable, InteractsWithSockets, SerializesModels;

    public $customer;

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

Then you need to create listeners that will perform the appropriate action (in this case – sending an e-mail message) if the event is triggered. Again, let us type in the console:

php artisan make:listener WelcomeNewCustomerListener

This time a new WelcomeNewCustomerListener class should be created in the app/Listeners directory. All you have to do in this case is put the code responsible for sending the e-mail in the handle() method. This method takes the $event parameter, which will contain the customer data transferred in the NewCustomerHasRegisteredEvent class constructor.

class WelcomeNewCustomerListener
{
    public function handle($event)
    {
        Mail::to($event->customer->email)->send(new WelcomeMail($event->customer));
    }
}

Now try to create by yourself one more listener responsible for sending an e-mail to the administrator, calling it e.g. NewCustomerAdminNotificationListener.

You have an event and listeners created. Finally, you have to bind everything together so that the appropriate listeners are launched when the appropriate event is triggered. In order to do this, add your event and listeners to the $listen array in the App\Providers\EventServiceProvider class:

class EventServiceProvider extends ServiceProvider
{
    protected $listen = [
        NewCustomerHasRegisteredEvent::class => [
            WelcomeNewCustomerListener::class,
            NewCustomerAdminNotificationListener::class,
        ],
    ];
}

Pipelines

The last, third example for today will be slightly different. You already know from the first example that in this case you should start by creating a new controller, e.g. CustomersQueryController, but you will not stop there. The code contained in the method is not bad; however, this can be done in a better way. In addition, you will use a function that is used quite often in Laravel, but there is not much information about it in the official documentation.

Pipelines are nothing more than a series of steps/tasks that you want to perform one by one. This will be perfect in the case where, depending on what data the user wants to obtain, you have to go through a series of steps to build an appropriate query using Eloquent.

So, let us start by moving the getQUeryResults method to a separate controller and renaming it:

class CustomersQueryController extends Controller
{
    public function index()
    {
        $customers = Customer::query();

        if (request()->has('name')) {
            $customers->where('name', request('name'));
        }

        if (request()->has('active')) {
            $customers->where('active', request('active'));
        }

        if (request()->has('sort')) {
            $customers->orderBy('name', request('sort'));
        }

        $customers->get();

        return $customers;
    }
}

Next, in the app directory, create a new QueryFitlers directory and three classes in it: name, active, sort. Each for one search parameter. This class will contain one handle() method, which will take two parameters: $request and $next. Now I will present the active class as an example. For practice, try to prepare the other two classes yourself.

class Active
{
    public function handle($request, \Closure $next)
    {

    }
}

In the handle method, you should put the condition opposite to the one that was in the controller so far. If the request does not contain an 'active' then go to the next step in the pipeline. Otherwise, add the appropriate condition to the builder and proceed further:

public function handle($request, \Closure $next)
{
    if (!request()->has('active')) {
        return $next($request);
    }

    $builder = $next($request);

    return $builder->where('active', request('active'));
}

Once you have prepared the other two classes in a similar way, all you have to do is bind everything together. So, let us go back to the controller and put all the above-mentioned classes through the pipeline:

public function index()
{
    $customers = app(Pipeline::class)
        ->send(Customer::query())
        ->through([
            \App\QueryFilters\Active::class,
            \App\QueryFilters\Name::class,
            \App\QueryFilters\Sort::class,
        ])
        ->thenReturn();

    return $customers->get();
}

Summary

In this article, I have presented three ways to extract logic from a controller. Of course, you do not have to limit yourself to just these – there are many more ways. You can read about useful Laravel functions that not everyone knows about.

So, remember to keep as little code as possible in the controller. This code should contain general logic only. You should put all the more detailed and confusing elements in a separate place. And always ask yourself: Is this in fact a task for the controller?