Be Open/Closed: how I removed else if and switch from my code

An awful switch statement you may want to refactor…

Do you ever wonder how to properly deal with multiple cases? Have you ever had to change your class implementation to take into account a new case?

As a freelancer, I often encounter code containing hard coded cases, which I will represent in this article with the code below:

class Dispatcher
{
  private $messageFactory;
  
  private $messageBus;
  
  public function __construct(MessageBus $bus, MessageFactory $factory)
  {
    $this->messageBus = $bus;
    $this->messageFactory = $factory;
  }
  
  public function dispatch(string $method, Subject $subject)
  {
    switch ($method):
      case 'ADD':
        $this->messageBus->dispatch(
          $this->messageFactory->add($subject)
        );
        break;
      case 'UPDATE':
        $this->messageBus->dispatch(
          $this->messageFactory->update($subject)
        );
        break;
      case 'DELETE':
        $this->messageBus->dispatch(
          $this->messageFactory->delete($subject)
        );
        break;
      default:
        throw new \DomainException(sprintf(
            'The %s method has not been configured for a dispatch.',
            $method
        ));
  }
}

Another way to write it would be to factorize the code in the dispatch method described in the example above:

public function dispatch(string $method, Subject $subject)
{
    $message = null;
    switch ($method):
      case 'ADD':
        $message = $this->messageFactory->add($subject);
        break;
      case 'UPDATE':
        $message = $this->messageFactory->update($subject);
        break;
      case 'DELETE':
        $message = $this->messageFactory->delete($subject);
        break;
      default:
        throw new \DomainException(sprintf(
            'The %s method has not been configured for a dispatch.',
            $method
        ));

    $this->messageBus->dispatch($message);
}

So what happens if we need to add another case?

• We need to change a class that was working well until now
• We will add some more code to an already quite long class
• If we add many cases in parallel (working as a team), we may encounter versioning conflicts
• One may forget to add the break instruction on one’s new case
• One of the case may not set the $message value, resulting in sending null to the message bus, probably fixed later by adding a if statement before the dispatch to test whether the message is null…

Removing a case is another problem…

OK then… How should we refactor?

public function dispatch(string $method, Subject $subject)
{
    $factories = [
      'ADD' => new class($this->messageFactory) {
        private $factory;
        public function __construct(MessageFactory $factory)
        {
          $this->factory = $factory;
        }
        public function __invoke(Subject $subject)
        {
          return $this->factory->add($subject);
        }
      },
      'UPDATE' => new class($this->messageFactory) {
        private $factory;
        public function __construct(MessageFactory $factory)
        {
          $this->factory = $factory;
        }
        public function __invoke(Subject $subject)
        {
          return $this->factory->update($subject);
        }
      },
      'DELETE' => new class($this->messageFactory) {
        private $factory;
        public function __construct(MessageFactory $factory)
        {
          $this->factory = $factory;
        }
        public function __invoke(Subject $subject)
        {
          return $this->factory->delete($subject);
        }
      },
    ];
    if (!isset($factories[$method])) {
      throw new \DomainException(sprintf(
        'The %s method has not been configured for a dispatch.',
        $method
      ));
    }
    $this->messageBus->dispatch($factories[$method]($subject));
}

For some it may not look better, does it? We still need some more work, adding return types, naming and moving classes, adding interfaces, and finally leveraging the dependency injection principle 😅

Let’s create an interface for the factory and associated classes:

interface Handler
{
    public function __invoke(Subject $subject): MessageInterface;
}
final class AddHandler implements Handler
{
    private $factory;
    public function __construct(MessageFactory $factory)
    {
        $this->factory = $factory;
    }
    public function __invoke(Subject $subject): MessageInterface
    {
        return $this->factory->add($subject);
    }
}

We can now provide an array of handlers to the initial class:

class Dispatcher
{
  private $messageFactory;
  
  private $messageBus;
  private $handlers;

  public function __construct(MessageBus $bus, MessageFactory $factory, array $handlers)
  {
    $this->messageBus = $bus;
    $this->messageFactory = $factory;
    $this->handlers = $handlers;
  }
  
  public function dispatch(string $method, Subject $subject)
  {
    if (!isset($this->handlers[$method]) || !$this->handlers[$method] instanceof Handler) {
      throw new \DomainException(sprintf(
        'The %s method has not been configured for a dispatch.',
        $method
      ));
    }
    $this->messageBus->dispatch($this->handlers[$method]($subject));
  }
}

Let’s wrap up:

• Adding a new handler is something that can be done without changing the class (just add a new object of type Handler to the associative array of handlers)
• Most of the interaction between contracts is typed, so less subject to errors (such as forgetting a break in a switch)

It can be massively improved:

• Instead of a string for the method, we could use an object
• Instead of an associative array (dictionary), we could use a list of Handler, allowing us to leverage the splat operator (…) to type a bit more and remove the second part of the if in the dispatch method (!$this->handlers[$method] instanceof Handler). It would require to either have a support method in the handler or some way to identify which handler is responsible for what…

Also to be noted: another solution exists using a pipeline, if you’re interested just say so and I’ll do a follow up article!

Challenge: refactor the following class 😃

vendor/guzzlehttp/psr7/src/functions.php:

/**
 * Create a new stream based on the input type.
 *
 * Options is an associative array that can contain the following keys:
 * - metadata: Array of custom metadata.
 * - size: Size of the stream.
 *
 * @param resource|string|null|int|float|bool|StreamInterface|callable|\Iterator $resource Entity body data
 * @param array                                                                  $options  Additional options
 *
 * @return StreamInterface
 * @throws \InvalidArgumentException if the $resource arg is not valid.
 */
function stream_for($resource = '', array $options = [])
{
    if (is_scalar($resource)) {
        $stream = fopen('php://temp', 'r+');
        if ($resource !== '') {
            fwrite($stream, $resource);
            fseek($stream, 0);
        }
        return new Stream($stream, $options);
    }

    switch (gettype($resource)) {
        case 'resource':
            return new Stream($resource, $options);
        case 'object':
            if ($resource instanceof StreamInterface) {
                return $resource;
            } elseif ($resource instanceof \Iterator) {
                return new PumpStream(function () use ($resource) {
                    if (!$resource->valid()) {
                        return false;
                    }
                    $result = $resource->current();
                    $resource->next();
                    return $result;
                }, $options);
            } elseif (method_exists($resource, '__toString')) {
                return stream_for((string) $resource, $options);
            }
            break;
        case 'NULL':
            return new Stream(fopen('php://temp', 'r+'), $options);
    }

    if (is_callable($resource)) {
        return new PumpStream($resource, $options);
    }

    throw new \InvalidArgumentException('Invalid resource type: ' . gettype($resource));
}