Trolled 2

Continuing on from a previous rant, a while ago I found myself fighting yet another losing battle about object oriented programming.

This time, it seems that “object oriented” actually means “I defined my own classes”. The context was a message queue handler, which a colleague had designed the architecture for. It looked something like this:

The message queue, dispatcher, etc. were all in place, and I was asked to implemented some the handlers. Here’s a (slightly simplified) example of what the code looked like:

• Each Handler implements this interface:

interface MessageHandler {
  function handle($message);
}

• The Handlers themselves looked like this (I’ve replaced the real work with placeholder calls to imaginary doStuffWith functions):

class EmailHandler implements MessageHandler {

  private $from, $credentials;

  function __construct($from, $credentials) {
    $this->from        = $from;
    $this->credentials = $credentials;
  }

  function handle($message) {
    doEmailStuffWith($this->from, $this->credentials, $message);
  }
}

class OrderHandler implements MessageHandler {

  private $orderSystem;

  function __construct($orderSystem) {
    $this->orderSystem = $orderSystem;
  }

  function handle($message) {
    doOrderStuffWith($this->orderSystem, $message);
  }
}

class AlertHandler implements MessageHandler {

  private $smsSystem, $emailSystem, $dashboardSystem;

  function __construct($smsSystem, $emailSystem, $dashboardSystem) {
    $this->smsSystem       = $smsSystem;
    $this->emailSystem     = $emailSystem;
    $this->dashboardSystem = $dashboardSystem;
  }

  function handle($message) {
    doAlertStuffWith($this->smsSystem, $this->emailSystem, $this->dashboardSystem, $message);
  }
}

// and so on

• The Handler classes were instantiated via the extremely complex Symfony framework. I don’t even begin to remember how any of that works, other than it consisted of YAML along these lines:

services:
  handlers.email:
    class:     EmailHandler
    arguments: [email.sender, email.credentials]

  handlers.order:
    class: OrderHandler
    arguments: [orders.connection]

  handlers.alert:
    class: AlertHandler
    arguments: [sms.sender, email.sender, dashboard.connection]

  # ...

• The meat of the dispatcher then looked like the following:

$this->get('handlers.' . $message->type);
$handler->handle($message);

So what’s the problem? Well, we’ll ignore Symfony’s absurd softcoding inner-platform for instantiating classes, and use of string matching to reimplement dynamic dispatch, and just focus on the Handler interface itself:

interface MessageHandler {
  function handle($message);
}

This tells us that each MessageHandler object only needs to contain a single method, handle. In which case, we might ask, why have the container at all? Why have $handler objects which we can only do one thing to? Having to call the handle method on $handler means that the $handler only exists to get in the way of us calling the code we care about. Why not remove the middle man, and make $handler be the code we care about?

To do this, we can use PHP’s __invoke method, in which case our code would become:

class EmailHandler implements MessageHandler {

  private $from, $credentials;

  function __construct($from, $credentials) {
    $this->from        = $from;
    $this->credentials = $credentials;
  }

  function __invoke($message) {
    doEmailStuffWith($this->from, $this->credentials, $message);
  }
}

class OrderHandler implements MessageHandler {

  private $orderSystem;

  function __construct($orderSystem) {
    $this->orderSystem = $orderSystem;
  }

  function __invoke($message) {
    doOrderStuffWith($this->orderSystem, $message);
  }
}

class AlertHandler implements MessageHandler {

  private $smsSystem, $emailSystem, $dashboardSystem;

  function __construct($smsSystem, $emailSystem, $dashboardSystem) {
    $this->smsSystem       = $smsSystem;
    $this->emailSystem     = $emailSystem;
    $this->dashboardSystem = $dashboardSystem;
  }

  function __invoke($message) {
    doAlertStuffWith($this->smsSystem, $this->emailSystem, $this->dashboardSystem, $message);
  }
}

// ...

$this->get('handlers.' . $message->type);
$handler($message);

So what about the MessageHandler interface? Without the handle method, it’s basically lost its only reason to exist. We could keep it around, for use as a type hint to ensure our code fits together properly, but I would argue that this interface misses the most important and tricky part of using the handlers: their constructors. PHP interfaces can’t enforce constructor signatures, and in any case all of the MessageHandler implementations have different constructor signatures!

So, I would say we ditch the MessageHandler interface completely, since it’s a classic case of not invented here syndrome. Instead, now that we’re using PHP’s standard __invoke method, we can also use PHP’s standard callable type hint instead.

Now that is, of course, making a mountain out of a molehill; however, there’s more! If we wade through the above code, we see that all of it is basically boilerplate for performing one task: dependency injection of the objects required to handle messages of that sort. Every class has the same structure:

class FooHandler {

  private $arg1, $arg2, ..., $argN;

  function __construct($arg1, $arg2, ..., $argN) {
    $this->arg1 = $arg1;
    $this->arg2 = $arg2;
    // ...
    $this->argN = $argN;
  }

  function __invoke($message) {
    doFooStuffWith($this->arg1, $this->arg2, ..., $this->argN, $message);
  }
}

The constructor arguments $arg1 to $argN are the dependencies, they’re stored in fields of the same name, which are declared private, and used inside __invoke. Since we’re now following PHP’s built-in standards, rather than inventing our own, we can use the special syntax provided for this pattern. It looks like this:

function($message) use ($arg1, $arg2, ..., $argN) {
  doFooStuffWith($arg1, $arg2, ..., $argN, $message);
}

This syntax defines a callable object (an instance of the Closure class), with an __invoke method accepting one argument ($message), with access to the private fields $arg1, $arg2, etc. whose body is the same doFooStuffWith call as above (although this time, we don’t need to prefix the fields with $this->).

This is much more succinct than manually reinventing everything, so there’s a lot less room for errors; in particular we don’t have to manually maintain a mapping between constructor argument names and member variable names. We also lose the manual private declarations, although we do gain a manual use declaration which is similar in overhead; I would still argue that this is an improvement, since forgetting a private declaration will leak implementation details and be tricky to spot, whilst forgetting a use declaration does the opposite and will be spotted immediately (with a “no such variable” error in the method body).

To provide the values of $argN, we can replace the use of __construct with a factory. One way would be to use a factory method, like this:

class FooHandler {
  public static function mkHandler($arg1, $arg2, ..., $argN) {
    return function($message) use ($arg1, $arg2, ..., $argN) {
             doFooStuffWith($arg1, $arg2, ..., $argN, $message);
           };
  }
}

This kind of factory class is supported by Symfony, but personally I think it’s yet more useless boilerplate. This class is just a container for a single method, like the original MessageHandler interface, so again we could replace its instances with callable objects instead. However, it’s even worse this time, since the method is static, which means we don’t need the class at all, so we can just pull out the function and write it standalone, like this:

function FooHandler($arg1, $arg2, ..., $argN) {
  return function($message) use ($arg1, $arg2, ..., $argN) {
           doFooStuffWith($arg1, $arg2, ..., $argN, $message);
         };
}

Now the entire original code has been boiled down to the following, with much less boilerplate, much less wheel-reinventing, much less architectural astronautics, much less time spent coupling all of the abstractions together, much more focus on the actual handling code and much more direct paths when debugging:

function EmailHandler($from, $credentials) {
  return function($message) use ($from, $credentials) {
           doEmailStuffWith($from, $credentials, $message);
         };
}

function OrderHandler($orderSystem) {
  return function($message) use ($orderSystem) {
           doOrderStuffWith($orderSystem, $message);
         };
}

function AlertHandler($smsSystem, $emailSystem, $dashboardSystem) {
  return function($message) use ($smsSystem, $emailSystem, $dashboardSystem) {
           doAlertStuffWith($smsSystem, $emailSystem, $dashboardSystem, $message);
         };
}

// and so on

$this->get('handlers.' . $message->type);
$handler($message);

However, when I brought up the fact that we could throw all of this junk away and use instances of PHP’s standard Closure class, using the built-in syntax to wire everything together rather than doing it all manually, I was told “that’s not OO”, that we should be “using classes and instances” and all of this cruft was required “in case we need to extend it in the future”.

At which point I wondered why we were using PHP’s built-in strings; what if we need to extend them in the future?

This is just one example of a common theme I’ve seen regarding what is or isn’t OO, which seems to imply that OOP means defining a bunch of classes, writing procedural code in their methods, and spending a huge amount of time trying to connect them all together. Apparently using the classes and objects already provided by the language, in the ways they were intended, doesn’t count as OOP :/