Refactoring Advanced Eloquent Filters To Collections

Amo Chohan wrote a very good article detailing a technique he uses to clean up controllers when dealing with complex search queries in eloquent.

I’m also enjoying Adam Wathan‘s latest release: Refactoring to Collections – a read guaranteed to make you smarter.

Amo really nailed the functionality here – but I think the final class presented could use some of Adam’s magic to make it just a little bit better. Let’s be honest – I’ve just been looking for an excuse to try out some of Adam’s techniques…

Below is the original class, and then my refactors.

Removing Static

Since all methods in this class are static, we can’t keep track of state. We end up passing around the variables that we’re using (the Request and Builder in this instance) to every method.

By adding a private constructor, we can keep the Api of the class the same, but internally gain the benefits of working with an object!

I’ve refactored the apply method to create a new instance of the object, and then call the appropriate methods to return the desired results. Since we don’t need the actual Request object anywhere else, I’ve gone ahead and created a collection out of the inputs for future use.

Introducing a Null Object

Currently we’re looping through the request inputs, and checking to see if we can find an existing filter to apply:

One way to get rid of that conditional is by introducing a null object that effectively accepts the query, but does nothing with it.

The above method allows us to refactor the applyDecoratorsFromRequest method (that I’ve renamed to applyFilters) into the following:

Since our filters are now a collection, we have access to the each method which I’ve used here instead. The getFilterFor method will always return a valid class that we can use – even if it is just a null object – allowing us to forego any conditionals!

I’m returning  $this just for convenience’s sake to allow me to chain my method calls.

Alternatively…

A refactor to my own method getFiltersFor could also more heavily make use of collections, and we can end up with a solution that completely eliminates loops and conditionals:

We’ve reworked the flow to do the following:

  • Map all filter names to class names
  • Filter out class names that aren’t implemented
  • Map all class names into new instances
  • Get the appropriate filter object, or return a new NullFilter if it doesn’t exist

In this case it’s debatable as to whether this second refactor adds any value – but an interesting solution nonetheless.

Moving On

For those who suffer from long and complicated index methods on your controllers, I urge you to read the original article by Amo.

As a side note, all of the refactors that I performed on the class (creating a private instance, introducing a null object, and refactoring to collections) were all lessons learned from Adam Wathan’s Refactoring to Collections book.

Happy coding!

Share: Share on FacebookTweet about this on TwitterShare on LinkedIn
  • StormShadow

    Michael thanks so much for this! I too had read both Amo’s original post and Adam’s excellent book and was sitting in a waiting room today thinking about what applying Adam’s methodology to Amo’s code would look like. Cheers!

  • znack

    Splendid!
    I just want your suggestion on how to implement this approach with Repository pattern?
    My app has all related methods in Repositories, such as filterByType or ByKeyword and so on:

    use IlluminateDatabaseEloquentModel;
    use IlluminateDatabaseEloquentBuilder;
    use IlluminateHttpRequest;

    abstract class AbstractRepo implements AbstractInterface
    {
    use FilterTrait;

    public $model;
    protected $filters;
    protected $query;

    private function __construct(Request $request, Builder $query, Model $model)
    {
    $this->model = $model;
    $this->filters = collect($request->all());
    $this->query = $query;
    }

    public function byType($type)
    {
    $this->model->where(‘type’,$type);
    }

    I have an issue of how to put data to cache.
    For now i think it should be like this:

    private function getResults(Builder $query)
    {
    $name = array_key($this->request);

    return Cache::remember($name, 10080, function(){
    $query->count() > 1 ? $query->paginate(15) : $query->get();
    }
    }

    Thank you for your help in advance

  • Felisha Banes

    Nice analysis – I learned a lot from the info – Does someone know if my assistant could possibly find a sample Personal Financial Statement version to fill out ?