simplify nested if statements

Question

I'm implementing a search functionality and based on the query parameter i use a different class to search.

class Search { 

    public function getResults()
    {
        if (request('type') == 'thread') {
                $results = app(SearchThreads::class)->query();
        } elseif (request('type') == 'profile_post') {
                $results = app(SearchProfilePosts::class)->query();
        } elseif (request()->missing('type')) {
                $results = app(SearchAllPosts::class)->query();
     }

}

Now when i want to search threads i have the following code.

class SearchThreads{

        public function query()
        {
            $searchQuery = request('q');
            $onlyTitle = request()->boolean('only_title');

            if (isset($searchQuery)) {
                if ($onlyTitle) {
                    $query = Thread::search($searchQuery);
                } else {
                    $query = Threads::search($searchQuery);
                }
            } else {
                if ($onlyTitle) {
                    $query = Activity::ofThreads();
                } else {
                    $query = Activity::ofThreadsAndReplies();
                }
            }
        }

}

To explain the code.

If the user enters a search word ( $searchQuery) then use Algolia to search, otherwise make a database query directly.

• If the user enters a search word

  1. Use the Thread index if the user has checked the onlyTitle checkbox
  2. Use the Threads index if the user hasn't checked the onlyTitle checkbox

• If the user doesn't enter a search word

  1. Get all the threads if the user has checked the onlyTitle checkbox
  2. Get all the threads and replies if the user hasn't checked the onlyTitle checkbox

Is there a pattern to simplify the nested if statements or should i just create a separate class for the cases where

1. a user has entered a search word
2. a user hasn't entered a search word

And inside each of those classes to check if the user has checked the onlyTitle checkbox

Answer 1

I would refactor this code to this:

Leave the request parameter to unify the search methods in an interface.

interface SearchInterface
{
    public function search(\Illuminate\Http\Request $request);
}

class Search {

    protected $strategy;

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

    public function getResults(\Illuminate\Http\Request $request)
    {
        return $this->strategy->search($request);
    }
}

class SearchFactory
{
    private \Illuminate\Contracts\Container\Container $container;

    public function __construct(\Illuminate\Contracts\Container\Container $container)
    {
        $this->container = $container;
    }

    public function algoliaFromRequest(\Illuminate\Http\Request  $request): Search
    {
        $type = $request['type'];
        $onlyTitle = $request->boolean('only_title');
        if ($type === 'thread' && !$onlyTitle) {
            return $this->container->get(Threads::class);
        }

        if ($type === 'profile_post' && !$onlyTitle) {
            return $this->container->get(ProfilePosts::class);
        }

        if (empty($type) && !$onlyTitle) {
            return $this->container->get(AllPosts::class);
        }

        if ($onlyTitle) {
            return $this->container->get(Thread::class);
        }

        throw new UnexpectedValueException();
    }

    public function fromRequest(\Illuminate\Http\Request $request): Search
    {
        if ($request->missing('q')) {
            return $this->databaseFromRequest($request);
        }
        return $this->algoliaFromRequest($request);
    }

    public function databaseFromRequest(\Illuminate\Http\Request $request): Search
    {
        $type = $request['type'];
        $onlyTitle = $request->boolean('only_title');
        if ($type === 'thread' && !$onlyTitle) {
            return $this->container->get(DatabaseSearchThreads::class);
        }

        if ($type === 'profile_post' && !$onlyTitle) {
            return $this->container->get(DatabaseSearchProfilePosts::class);
        }

        if ($type === 'thread' && $onlyTitle) {
            return $this->container->get(DatabaseSearchThread::class);
        }

        if ($request->missing('type')) {
            return $this->container->get(DatabaseSearchAllPosts::class);
        }

        throw new InvalidArgumentException();
    }
}


final class SearchController
{
    private SearchFactory $factory;

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

    public function listResults(\Illuminate\Http\Request $request)
    {
        return $this->factory->fromRequest($request)->getResults($request);
    }
}

The takeaway from this is it is very important to not involve the request in the constructors. This way you can create instances without a request in the application lifecycle. This is good for caching, testability and modularity. I also don't like the app and request methods as they pull variables out of thin air, reducing testability and performance.

Answer 2

class Search 
{
     public function __construct(){
             $this->strategy = app(SearchFactory::class)->create();
         }

        public function getResults()
        {
             return $this->strategy->search();
         }
}

class SearchFactory
{
    public function create()
    {
        if (request()->missing('q')) {
            return app(DatabaseSearch::class);
        } else {
            return app(AlgoliaSearch::class);
        }

    }
}

class AlgoliaSearch implements SearchInterface
{

    
    public function __construct()
    {
        $this->strategy = app(AlgoliaSearchFactory::class)->create();
    }
    public function search()
    {
        $this->strategy->search();
    }
}

class AlgoliaSearchFactory
{

    public function create()
    {
        if (request('type') == 'thread') {
            return app(Threads::class);
        } elseif (request('type') == 'profile_post') {
            return app(ProfilePosts::class);
        } elseif (request()->missing('type')) {
            return app(AllPosts::class);
        } elseif (request()->boolean('only_title')) {
            return app(Thread::class);
        }
    }
}

Where the classes created in the AlgoliaSearchFactory are algolia aggregators so the search method can be called on any of those classes.

Would something like this make it cleaner or even worse ?

Right now i have strategies that have strategies which sounds too much to me.

Answer 3

I have tried to implement a good solution for you, but I had to make some assumptions about the code.

I decoupled the request from the constructor logic and gave the search interface a request parameter. This makes the intention clearer than just pulling the Request from thin air with the request function.

final class SearchFactory
{
    private ContainerInterface $container;

    /**
     * I am not a big fan of using the container to locate the dependencies.
     * If possible I would implement the construction logic inside the methods.
     * The only object you would then pass into the constructor are basic building blocks,
     * independent from the HTTP request (e.g. PDO, AlgoliaClient etc.)
     */
    public function __construct(ContainerInterface $container)
    {
        $this->container = $container;
    }

    private function databaseSearch(): DatabaseSearch
    {
        return // databaseSearch construction logic
    }

    public function thread(): AlgoliaSearch
    {
        return // thread construction logic
    }

    public function threads(): AlgoliaSearch
    {
        return // threads construction logic
    }

    public function profilePost(): AlgoliaSearch
    {
        return // thread construction logic
    }

    public function onlyTitle(): AlgoliaSearch
    {
        return // thread construction logic
    }

    public function fromRequest(Request $request): SearchInterface
    {
        if ($request->missing('q')) {
            return $this->databaseSearch();
        }

        // Fancy solution to reduce if statements in exchange for legibility :)
        // Note: this is only a viable solution if you have done correct http validation IMO
        $camelCaseType = Str::camel($request->get('type'));
        if (!method_exists($this, $camelCaseType)) {
            // Throw a relevent error here
        }

        return $this->$camelCaseType();
    }
}

// According to the code you provided, algoliasearch seems an unnecessary wrapper class, which receives a search interface, just to call another search interface. If this is the only reason for its existence, I would remove it
final class AlgoliaSearch implements SearchInterface {
    private SearchInterface $search;

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

    public function search(Request $request): SearchInterface {
        return $this->search->search($request);
    }
}

I am also not sure about the point of the Search class. If it only effectively renames the search methods to getResults, I am not sure what the point is. Which is why I omitted it.

Answer 4

I had to write all this to make the problem understandable.

The SearchFactory takes all the required parameters and based on these parameters, it calls either AlgoliaSearchFactory or DatabaseSearchFactory to produce the final object that will be returned.

class SearchFactory
{
    protected $type;
    protected $searchQuery;
    protected $onlyTitle;
    protected $algoliaSearchFactory;
    protected $databaseSearchFactory;

    public function __construct(
        $type,
        $searchQuery,
        $onlyTitle,
        DatabaseSearchFactory $databaseSearchFactory,
        AlgoliaSearchFactory $algoliaSearchFactory
    ) {
        $this->type = $type;
        $this->searchQuery = $searchQuery;
        $this->onlyTitle = $onlyTitle;
        $this->databaseSearchFactory = $databaseSearchFactory;
        $this->algoliaSearchFactory = $algoliaSearchFactory;
    }

    public function create()
    {
        if (isset($this->searchQuery)) {
            return $this->algoliaSearchFactory->create($this->type, $this->onlyTitle);
        } else {
            return $this->databaseSearchFactory->create($this->type, $this->onlyTitle);
        }
    }
}

The DatabaseSearchFactory based on the $type and the onlyTitle parameters that are passed from the SearchFactory returns an object which is the final object that needs to be used in order to get the results.

class DatabaseSearchFactory
{
    public function create($type, $onlyTitle)
    {
        if ($type == 'thread' && !$onlyTitle) {
            return app(DatabaseSearchThreads::class);
        } elseif ($type == 'profile_post' && !$onlyTitle) {
            return app(DatabaseSearchProfilePosts::class);
        } elseif ($type == 'thread' && $onlyTitle) {
            return app(DatabaseSearchThread::class);
        } elseif (is_null($type)) {
            return app(DatabaseSearchAllPosts::class);
        }
    }
}

Same logic with DatabaseSearchFactory

class AlgoliaSearchFactory
{
    public function create($type, $onlyTitle)
    {
        if ($type == 'thread' && !$onlyTitle) {
            return app(Threads::class);
        } elseif ($type == 'profile_post' && !$onlyTitle) {
            return app(ProfilePosts::class);
        } elseif (empty($type) && !$onlyTitle) {
            return app(AllPosts::class);
        } elseif ($onlyTitle) {
            return app(Thread::class);
        }
    }
}

The objects that are created by AlgoliaSearchFactory have a method search which needs a $searchQuery value

interface AlgoliaSearchInterface
{
    public function search($searchQuery);
}

The objects that are created by DatabaseSearchFactory have a search method that doesn't need any parameters.

interface DatabaseSearchInterface
{
    public function search();
}

The class Search now takes as a parameter the final object that is produced by SearchFactory which can either implement AlgoliaSearchInterface or DatabaseSearchInterface that's why I haven't type hinted

The getResults method now has to find out the type of the search variable ( which interface it implements ) in order to either pass the $searchQuery as a parameter or not.

And that is how a controller can use the Search class to get the results. class Search { protected $strategy;

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

    public function getResults()
    {
        if(isset(request('q')))
        {
            $results = $this->strategy->search(request('q'));
        }
        else
        {
            $results = $this->strategy->search();
        }
    }
}


class SearchController(Search $search)
{
    $results = $search->getResults();
}

According to all of @Transitive suggestions this is what I came up with. The only thing that I cannot solve is how to call search in the getResults method without having an if statement.