1

I have an class SearchEngine which can have different behaviors and thus I need to pass a search strategy object. Currently I have a setter method in the SearchEngine, which consists of a SearchStrategyFactory

class SearchEngine
{
    protected $strategy;

    public function setStrategy($type)
    {
          $this->strategy = app(SearchStrategyFactory::class)->create($type);
    }

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

}

My question is if it is a bad design to use a factory class within a method to get an object.

I know that I should better pass the strategy through the constructor or as a parameter in the setter method maybe.

But the way I have it know, I’m avoiding a number of nested if statements.

Of course any suggestion is appreciated.

Orestis uRic
  • 347
  • 1
  • 6
  • 11
  • So this question is probably a better fit for https://softwareengineering.stackexchange.com/. But while it's here: shouldn't your object always have a strategy anyway? In which case, a constructor-injected value would make most sense. And I mean injecting the strategy object itself (the constructor can always have it as an optional argument and generate one using a factory if it's null, if you really need that). – Jeto Oct 09 '20 at 19:11
  • If you want the same instance of SearchEngine to be able to search with different strategies, could you just make the search method take an object implementing a strategy interface, or are there other reasons that SearchEngine needs to have a strategy rather than just using it for search()? On the other hand, if each instance of SearchEngine only uses one strategy, constructor injection seems fine. – Don't Panic Oct 09 '20 at 20:05
  • @Jeto Yes the SearchEngine will always have a strategy. First I instantiate the SearchEngine object and then I set the strategy. Regarding the injection of the strategy itself, the reason I'm doing it this way now is because I have 3 variables that can determine the final SearchStrategy, which means many if statements and some of them are nested. – Orestis uRic Oct 09 '20 at 20:41
  • @Jeto In order to avoid having nested if statements, I tried to break the if statements into separate family of strategies. The SearchEngine exists to break the if statements. So based on 1 of the 3 variables, I create a SearchEngine ( I have 2 types of SearchEngines) , this way I get rid of the nested if statements. Then based on the other 2 variables ( these 2 variables are inside the SearchStrategyFactory) I decide which SearchStrategy to use for the given SearchEngine. – Orestis uRic Oct 09 '20 at 20:42
  • @Don'tPanic Each instance of SearchEngine can use different strategies based on the user's request. If the user wants to search Apples, then AppleStrategy is used and so on. – Orestis uRic Oct 09 '20 at 20:42

1 Answers1

1

I am not a fan of factory methods if a simple dependency injection is a viable alternative.

Even worse, you are not only using a factory pattern in your own SearchEngine class, but you are also using the locator pattern to create it.

I consider this code bad design because:

  • By making your strategy mutable, you make it hard to reason about the current method used by the SearchEngine.
  • By using the locator pattern, you are introducing a second dependency. That of the strategy as well as the container. All while losing modularity as the construction logic is now partly tied to your own SearchEngine implementation.

I suggest you use dependency injection. Make the strategy immutable. If you need the strategy to be mutable, use a setter which accepts a strategy instance, instead of a strategy class string.

If you then notice during the construction you are having a bunch of branching logic to decide the instantiation; you probably are not utilizing DI to its full potential. Moving the logic to a setter to get rid of the if statements will probably introduce even worse bugs as the branching will instead happen anywhere within your codebase, making it harder to reason about the current strategy.

Ghlen
  • 659
  • 4
  • 14
  • Thanks for the answer. To illustrate the initial issue that I had with the if statements you can find it here https://stackoverflow.com/questions/64247112/simplify-nested-if-statements?noredirect=1#comment113612434_64247112 – Orestis uRic Oct 09 '20 at 20:45
  • If you take a look at my other question ( on the given link ), I have 3 variables that determine the SearchStrategy. The $searchQuery if statement is replaced by the SearchEngine. This way i break a nested if statements and I create a SearchStrategyFactory that takes care of the other 2 variables in order to determine the SearchStrategy – Orestis uRic Oct 09 '20 at 20:49
  • Dear @OrestisuRic, I looked at the code and apart from my prior comments about dependency injection, I do not see a problem with these if statements. The code is very obvious and understandable. I do not see how you could use any other technique without significantly reducing legibility and maintainability. I understand you might not like if statements, but tieing polymorphism into dynamic problems is also not a good fit IMO, and 4 if statements will fit in even a monkeys head – Ghlen Oct 10 '20 at 16:54
  • 1
    Dear @Transitive, thanks for the response again. Maybe I’m overcomplicating things for no reason then. ( it fits in a monkey’s head lol ) – Orestis uRic Oct 10 '20 at 17:09
  • Dear @Transitive from the code that you saw, would it make sense to call a factory within another factory ( of course this just hides the if statements in another class ) – Orestis uRic Oct 10 '20 at 18:57
  • I think I would change the create method from AlgoliaSearchFactory to a method called: fromRequest, which receives the request as a parameter. Then I would make 3 new methods, each of which creating threads, profilePosts, allPosts respectively. fromRequest would then create the Algoliasearch depending on the provided request in fromRequest. No new factory needed IMO. That way you can get rid of the locator pattern as well. That being said, there is nothing wrong with factories calling other factories. – Ghlen Oct 10 '20 at 19:08
  • You lost me. The AlgoliaSearch is the class that uses the AlgoliaSearchFactory to create Threads, ProfilePosts etc. You said that you would use AlgoliaSearchFactory to create the AlgoliaSearch which is the other way around. Would it be easy for you to provide an answer on that question so I can better understand with code rather ran text. – Orestis uRic Oct 10 '20 at 21:12
  • I added an implementation there. If you feel this answer was sufficient to solve this thread, please do not forget to mark it as the solution. – Ghlen Oct 11 '20 at 11:39