'Lean controller'/'Business logic on service' and filtering, sorting, pagination methods on controllers

Question

I'm currently reading about "Lean controllers"/"Business logic on services" and trying to refactor some legacy code. However I am struggling to apply what is taught in the tutorials to real code.

Online examples are simple. But in real code, I have controllers that need to use filtering, sorting, pagination (and the fact that some of these elements are part of the Eloquent query makes me even more stuck):

Legacy code

class EmployeeController extends Controller
{
    public function index(Request $request)
    {
        $employeesQuery = Employee::with(['gender', 'birthState', 'documentType', 'maritalStatus', 'addressState', 'user']);

        //filters
        $filters = ModelFilterHelpers::buildFilters($request, Employee::$acceptedFilters);
        $employeesQuery = $employeesQuery->AcceptRequest(Employee::$acceptedFilters)->filter();

        //sort
        $employeesQuery = $employeesQuery->sortable(['updated_at' => 'desc']);

        //get paginate and add querystring on paginate links
        $employees = $employeesQuery->paginate(10);
        $employees->withQueryString();

        return view('employee.index', compact('employees', 'filters'))->with('i', (request()->input('page', 1) - 1) * 10);
    }

Keeping everything together within just one service function feels wrong to me and separating into other functions seems complex to me.

What I have so far is:

App\Http\Controllers\EmployeeController.php

class EmployeeController extends Controller
{
    public function index(Request $request)
    {
        //filters
        $filters = ModelFilterHelpers::buildFilters($request, Employee::$acceptedFilters);
        
        $employees = EmployeeService::getEmployees();

        return view('employee.index', compact('employees', 'filters'))->with('i', (request()->input('page', 1) - 1) * 10);
    }

App\Services\EmployeeService.php

class EmployeeService
{
    public static function getEmployees() : LengthAwarePaginator
    {
        $employeesQuery = Employee::with(['gender', 'birthState', 'documentType', 'maritalStatus', 'addressState', 'user']);

        $employeesQuery = $employeesQuery->AcceptRequest(Employee::$acceptedFilters)->filter();

        //sort
        $employeesQuery = $employeesQuery->sortable(['updated_at' => 'desc']);

        //get paginate and add querystring on paginate links
        $employees = $employeesQuery->paginate(10);

        return $employees->withQueryString();
    }

App\Models\Employee.php (mdfst13 requested on comments)

class Employee extends Model
{
    use HasFactory;
    use Sortable;
    use employeeFilter, Filterable;

    protected $fillable = [
        'name',
        'email',
    ];

    public $sortable = [
        'id',
        'name',
        'user.email',
        'created_at',
        'updated_at'
    ];

    public static $acceptedFilters = [
        'name_contains',
        'user_email_contains'
    ];

    public function gender()
    {
        return $this->belongsTo(Gender::class);
    }

    public function birthState()
    {
        return $this->belongsTo(State::class);
    }

    public function documentType()
    {
        return $this->belongsTo(DocumentType::class);
    }

    public function maritalStatus()
    {
        return $this->belongsTo(MaritalStatus::class);
    }

    public function addressState()
    {
        return $this->belongsTo(State::class);
    }

    public function user()
    {
        return $this->hasOne(User::class);
    }

So I ask:

• Is this separation correct?
• Is it okay to have filtering, sorting, pagination elements in one service method?

Answer 1

Is this separation correct?

While helper methods/classes (e.g. EmployeeService::getEmployees()) are nice to have, I would only do that if the same actions are repeated in multiple places. If it is just moving five lines from a controller to another spot then it feels like its too much extra overhead (e.g. 6+ lines and an extra file).

The index method only has seven lines of code (not counting comments). That doesn't seem very "fat". A method that has 15+ lines would be ideal for refactoring. A Local Scope on the model could be used to move some of the query modifications (e.g. filtering, sorting) if desired.

For example, instead of specifying the relations to include in the controller:

$employeesQuery = Employee::with(['gender', 'birthState', 'documentType', 'maritalStatus', 'addressState', 'user']);

A local scope could be created in the model:

class Employee extends Model
{
    ....//traits, properties, relations, etc.
    /**
     * @param Illuminate\Database\Eloquent\Builder $query
     * @return \Illuminate\Database\Eloquent\Builder
     */
    public function scopeWithListRelations(Builder $query)
    {
        return $query->with(['gender', 'birthState', 'documentType', 'maritalStatus', 'addressState', 'user']);
    }
}

Then the controller can use it:

$employeesQuery = Employee::withListRelations()->AcceptRequest(Employee::$acceptedFilters)->filter();

If the controllers have more than ~6-7 methods then consider breaking it up into separate controllers. While reading about "lean controllers" recently I found this post from 2019 which not only includes a link to Taylor Otwell's podcast about where to put code logic but also Adam Wathan’s talk at Laracon US 2017, called “CRUDdy By Design”. In that talk he describes ways to use standard verbs with new controller methods/actions instead of custom actions. Attempt to map actions to the resource actions:

• index
• show
• create
• store
• edit
• update
• destroy

For example, instead of a route tied to a controller method:

Route::post('/podcasts/{id}/update-cover-image', 'PodcastsController@updateCoverImage');

Have a PodcastCoverImageController with a simple update method to handle updating the cover image for the podcast:

Route::put('/podcasts/{id}/update-cover-image', 'PodcastCoverImageController@update');

And similarly, instead of having a Controller method subscribe - think about what that action does: it creates a subscription for a podcast. So a SubscriptionsController can be created with a store method for subscribing to a podcast (and destroy for unsubscribing to a podcast).

Is it okay to have filtering, sorting, pagination elements in one service method?

If that is what is needed to properly list the employees, it seems fine to have that in one method.

Answer 2

What you are trying to achieve can be done with https://github.com/spatie/laravel-query-builder.

If you are to work on something custom, you'd want to re-create (or extend) what laravel-query-builder does (and by the way, with your code you are on the right track.)

If we are to think in terms of SQL, to create a query you'd use WHERE, LIMIT and similar. The same logic can be applied here, let say we have a URL with the following params - /api/employees?where[name]=Joe Doe or /api/employees?limit=10&offset=10.

Within your query builder class, you'd want to have a generic support for those parameters and you can either whitelist things within the builder or within the model (the way you did).

An example implementation, when using a builder class, may look similar to below:

controller

public index(Request $request) {
  return EmployeeQueryBuilder($request->all())
    ->make()
    ->get();
}

EmployeeQueryBuilder

class EmployeeQueryBuilder extends BaseQueryBuilder {
  protected array $allowedWith = [...];
  protected int $defaultLimit = 10;
  protected int $maxLimit = 10;
  // whatever else you would want
  protected $model = Employee::class;
}

BaseQueryBuilder

abstract class QueryBuilder {
  // relevant property definitions

  public __constructor(array $data) {
    $this->data = $data;
  }

  public function make(): Builder {
    // ... loop through support parameters, such as where and limit
    $this->handleWhere();
    $this->handleLimit();
    $this->handleWith();

    ...
  }

  protected function handleWhere(): void {
    // validate $data['where'] and then...
    $this->query->where($data['where']);
  }
}