Is this code too brittle?

Question

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"

Answer 1

Hmm, well, I dont think it's too brittle. You dont need the strings though. You could simply use an ordered array since the naming corresponds to 0,1,2,3 anyway. If you are concerned about invalid strategies or classes being supplied, you could put some validation into the manager.

public function __construct() {
    $this->selectedStrategies = array(
        /* could add some default strategies */
    );
}
public function load(array $userStrategies) {
    for( $i=0; $i<3; $i++ ) {
        try {
            $rc = new ReflectionClass($userStrategies[$i]);
            if( $rc->implementsInterface('Iterator') ) {
                $this->selectedStrategies[$i] = new $userStrategies[$i];
            } else {
                throw new InvalidArgumentException('Not a Strategy');
            }
        } catch(ReflectionException $e) {
            throw new InvalidArgumentException('Not a Class');
        }
    }
}

And instead of calling the strategies with your associative keys, you simply

$this->selectedStrategies[0]->execute();

and so on.

---

Yet another approach would be to use

class StrategyCollection
{
    protected $strategies;

    public function __construct() {
        $this->strategies = new SplFixedArray(4);
    }

    public function add(IStrategy $strategy) {
        $this->strategies[] = $strategy;
        return $this;
    }
}

and then fill the Manager/Collection from the outside. With the typehint for IStrategy you can be sure only classes implementing the strategy interface end up in the manager. That saves you the somewhat expensive Reflection calls when creating the strategies. The SplFixedArray makes sure there is a Runtime Exception when you try to add more than four strategies.

---

On a sidenote, dont trust input from a selectbox. Just because a selectbox gives fixed options, doesnt mean malicious users cannot tinker with the request. All request data has to be sanitized and doublechecked.

Answer 2

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();

Answer 3

If the strategy functions don't need state, you could switch to a functional style of programming and replace the whole class with: call_user_func($strategy['first']); (second, etc). If concerned about the global namespace, they functions could be stored as static members of a class - ie call_user_func(array('Strategies', $strategy['first'])); You could then get a list of all valid strategies (for generating and testing the selectbox) using get_class_methods('Strategies');, which could simplify the code by having just the one global list of valid strategies.

If you do need state stored with the strategy functions - I might use some kind of caching call function - something like

function doStrategy($class) {
    static $selectedStrategies = array();

    if (!isset($selectedStrategies[$class])) {
        $selectedStrategies[$class] = new $class;
    }

    $Strategy = $selectedStrategies[$class];
    $Strategy->execute(); // some versions of PHP require two lines here
}

Of course you could still use a class over a function to do this as well :P.

The "Stringly Typed" epithet is not applicable for PHP as it is both weakly typed and already internally using strings to store symbols (class and function names, variables, etc). As such, for reflection the string data type is often the best fit. We won't go into what that means for the language as a whole.