One of the issues that should be addressed is that the invoker (the Session class in the post), is only extensible by actually extending the class. If we want to add custom methods to it, we have two options: make our own sub-class, which should be avoided when favoring composition over inheritance; or modifying the base class's constructor, which violates the open/closed principle.
Another thing that could be improved is that the invoker is violating the Single-Responsibility principle (which violates the "S" in SOLID.) It needs to know not just how to execute commands, but also how to construct them. As the list of available commands grows, and the constructors for each command become more complex, the invoker needs to contain more logic to build each command type.
Let's overcome some of these issues, and also make the code even more extensible. I'll use a simplified command invoker to demonstrate.
To start, let's remove the complexity of requiring the invoker know how to build each command object. A simple way to do this is the move the factory methods into the individual command classes. We'll also define a simple interface to ensure that our commands are executable. Here are two simple commands. Note that the constructor is not part of the interface, which allows the command classes to have completely different constructor method signatures.
interface Command { public static function register(); public function execute(); } class HelloCommand implements Command { protected $name; public static function register() { return function ($name) { return new HelloCommand($name); }; } public function __construct($name) { $this->name = $name; } public function execute() { echo "Hello, " . $this->name . "\n"; } } class PwdCommand implements Command { public static function register() { return function () { return new PwdCommand(); }; } public function execute() { echo "You are here: " . getcwd() . "\n"; } }Each command class has a static `register` method that returns an anonymous function which knows how to instantiate the class. We'll call this the "factory" function. In the example, the factory function signatures are the same as the constructors, but that is not necessary; as long as the factory function can construct a valid object, it can take whatever parameters it needs.
Our commands can construct themselves, so the invoker only has to know how to map its own method calls to the commands. Here is the simplified Invoker class:
class Invoker { protected $commandMap = array(); public function __construct() { $this->register('hello', 'HelloCommand'); $this->register('pwd', 'PwdCommand'); } public function __call($command, $args) { if (!array_key_exists($command, $this->commandMap)) { throw new BadMethodCallException("$command is not a registered command"); } $command = call_user_func_array($this->commandMap[$command], $args); $command->execute(); } public function register($command, $commandClass) { if (!array_key_exists('Command', class_implements($commandClass))) { throw new LogicException("$commandClass does not implement the Command interface"); } $this->commandMap[$command] = $commandClass::register(); } }The important bit is the `register` method. It checks that the registered class is indeed a Command, and thus, that it will have both an `execute` and a `register` method. We then invoke the `register` method, and store the factory function for that command.
In the magic `__call` method, we have to use `call_user_func_array` since we don't know the real signature of the called factory function. If we wanted, we could have the `__call` method return the command object instead of executing it. That would turn the Invoker into an Abstract Factory for command objects.
The main point is that Invoker never cares exactly how commands are constructed. Invoker now only has one responsibility: invoking commands.
We execute commands by calling Invoker methods with parameters that match the factory function signature of the command being executed:
$invoker = new Invoker(); $invoker->hello('Josh'); $invoker->pwd();By making the Invoker's `register` method public, we've also opened up the Invoker to allow a client to extend its functionality without having to modify the Invoker itself:
class CustomCommand implements Command { protected $foo; protected $bar; public static function register() { return function ($foo, $bar) { return new CustomCommand($foo, $bar); }; } public function __construct($foo, $bar) { $this->foo = $foo; $this->bar = $bar; } public function execute() { echo "What does it all mean? " . ($this->foo + $this->bar) . "\n"; } } $invoker->register('custom', 'CustomCommand'); $invoker->custom(40, 2);We can even let clients overwrite existing commands by re-registering with a custom class:
$invoker->register('pwd', 'CustomCommand'); $invoker->pwd(2, 40);If we don't want to allow this, we can build more checks into `register` that prevent overwriting built-in commands.
So now we have an extendable, flexible command invocation system that requires no modification to the actual command runner. This could also be used as the basis for a plug-in system.
Full sample code is available at http://gist.github.com/1610148
These are all valid directions to go towards, and as more commands are added I will indeed extract their creation logic, which would become a large part of the Session class. Note however that I need to build the commands "just in time", before they are called, so the contract would be a creational interface like a Factory or a Builder.
ReplyDeleteI'm waiting for the tests to show up the need for this extensions - for example adding custom commands won't probably be necessary as Selenium 2 only supports a fixed list.
Thanks for the reply! I wasn't suggesting that this is how you should do it with Selenium, I was just trying to see how the solution in your post could be extended in a general way. Obviously if there is no need for user-defined commands, the register method would be kept private to the Invoker.
ReplyDeleteI'm a bit confused why the commands aren't being built "just in time." No command object exists until the __call method, only the factory function. Also, the creation contract is enforced by the factory function or the command's constructor. Is there something I'm missing? Thanks again for the original post and reply.