Search code examples
phpstringstrategy-pattern

Is this code too brittle?


I need to create a strategy pattern where a user selects four strategies from a list of twenty or thirty unique strategy objects. The list of strategies will be expanded as the project matures, and users can change their selected strategy at any time.

I plan to store the strategy names they have selected as strings, and then use a method like this one to load the strategy classes that correspond to the strings they selected.

class StrategyManager { // simplified for the example
    public $selectedStrategies = array();
    public function __construct($userStrategies) {
        $this->selectedStrategies = array(
            'first'  => new $userStrategies['first'],
            'second' => new $userStrategies['second'],
            'third'  => new $userStrategies['third'],
            'fourth' => new $userStrategies['fourth']
        );
    }

    public function do_first() {
        $this->selectedStrategies['first']->execute();
    }

    public function do_second() {
        $this->selectedStrategies['second']->execute();
    }

    public function do_third() {
        $this->selectedStrategies['third']->execute();
    }

    public function do_fourth() {
        $this->selectedStrategies['fourth']->execute();
    }
}

I'm trying to avoid a large switch statement. My concern is that this seems kinda Stringly Typed. Is there a better way to accomplish this goal without using a conditional or a large switch statement?

BTW: The user does not input a string when selecting the four strategies. I would need to maintain a list of strings to present to the user in a select box, and add new ones to the list as I add new strategy objects.

Explanation
ircmaxell expressed a bit of confusion regarding what it is I'm trying to do. In the above example, the user has selected four strategies from a list, and they are passed to the StrategyManager constructor as an array of strings. The corresponding strategy objects are created and stored in an internal array, $this->selectedStrategies

"first", "second", "third" and "fourth" are the array keys of the internal array for the four different selected strategies. After the StrategyManager object has been built, the application uses the execute method of the four strategies during various moments over the life of the process.

So, in a nutshell... every time the application needs to execute the method of Strategy number "one" it does so, and the results are different depending on what strategy was selected by the user for Strategy "one"


Solution

  • Based on your comments and updates, I don't think that this code is too brittle. It will be harder to maintain if you either alter the call chain for a strategy type (do_one, do_two, etc) or add strategies. What I would instead recommend is using an abstract factory to create the "strategies". Then, in the code where you need the strategy, fetch the strategy object itself...

    The reason that I like this method better is two fold. First, it only creates the strategies on demand, so you're not building objects you don't need. Second, it encapsulates the users choice since that's the only place that needs to look it up (you could build it with dependency injection, but you'd need somewhere else to manage the building as well).

    class StrategyFactory {
    
        protected $strategies = array();
    
        //If you like getter syntax
        public function __call($method, $arguments) {
            $method = strtolower($method);
            if (substr($method, 0, 3) == 'get') {
                $strategy = substr($method, 3);
                return $this->getStrategy($strategy);
            }
            throw new BadMethodCallException('Unknown Method Called');
        }
    
        public function getStrategy($strategy) {
            if (isset($this->strategies[$strategy])) {
                return $this->strategies[$strategy];
            } elseif ($this->makeStrategy($strategy)) {
                return $this->strategies[$strategy];
            }
            throw new LogicException('Could not create requested strategy');
        }
    
        protected function makeStrategy($name) {
            //pick strategy from user input
            if ($strategyFound) {
                $this->strategies[$name] = new $strategy();
                return true;
            } else {
                return false;
            }
        }
    }
    

    Then, use like so:

    $strategy = $factory->getSomeStrategyName();
    $strategy->execute();
    

    or even with chaning:

    $factory->getSomeStrategyName()->execute();
    

    Or without magic methods:

    $factory->getStrategy('strategyName')->execute();