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.
<?php
namespace App\UserSearch;
use App\User;
use Illuminate\Http\Request;
use Illuminate\Database\Eloquent\Builder;
class UserSearch
{
public static function apply(Request $filters)
{
$query =
static::applyDecoratorsFromRequest(
$filters, (new User)->newQuery()
);
return static::getResults($query);
}
private static function applyDecoratorsFromRequest(Request $request, Builder $query)
{
foreach ($request->all() as $filterName => $value) {
$decorator = static::createFilterDecorator($filterName);
if (static::isValidDecorator($decorator)) {
$query = $decorator::apply($query, $value);
}
}
return $query;
}
private static function createFilterDecorator($name)
{
return return __NAMESPACE__ . '\\Filters\\' .
str_replace(' ', '',
ucwords(str_replace('_', ' ', $name)));
}
private static function isValidDecorator($decorator)
{
return class_exists($decorator);
}
private static function getResults(Builder $query)
{
return $query->get();
}
}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!
protected $filters;
protected $query;
private function __construct(Request $request, Builder $query)
{
$this->filters = collect($request->all());
$this->query = $query;
}
public static function apply(Request $request)
{
return (new self($request, User::query())->applyFilters()->getResults();
}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:
foreach ($request->all() as $filterName => $value) {
$decorator = static::createFilterDecorator($filterName);
if (static::isValidDecorator($decorator)) {
$query = $decorator::apply($query, $value);
}
}One way to get rid of that conditional is by introducing a null object that effectively accepts the query, but does nothing with it.
private function getFilterFor($name)
{
$filterClassName = $this->createFilterDecorator($name);
if (! class_exists($filterClassName)) {
return new NullFilter;
}
return new $filterClassName;
}The above method allows us to refactor the applyDecoratorsFromRequest method (that I’ve renamed to applyFilters) into the following:
private function applyFilters()
{
$this->filters->each(function ($value, $filterName) {
$this->getFilterFor($filterName)->apply($this->query, $value);
});
return $this;
}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:
private function getFilterFor($name)
{
return $this->filters->map(function ($value, $filterName) {
return $this->createFilterDecorator($filterName);
})->filter(function ($filterClassName) {
return class_exists($filterClassName);
})->map(function ($filterClassName) {
return new $filterClassName;
})->get($name, new NullFilter);
}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!
