Code Reviewing a Mug

Posted at 10:00, 14th July 2017

Image credit: Cal Harding

So, in the past couple of days, a well-meaning friend posted the following image into a chat channel populated 50% by software engineers:

What they assumed was a nice nod to the coders soon became something of a nightmare as we all just... code reviewed the mug.

I'm sure if you've worked in software development for a while, you've seen this kind of effect before - someone publishes code or pseudo-code on a promotional item or in marketing material as a way to establish geek cred, or perhaps to attract talent for a hiring campaign. If you've worked with teams actively hiring, you might even have seen your own employers attempting this - and are probably aware that the exact same reaction to my friend's mug shot is bound to happen both internally and externally around such efforts.

Well, in the pursuit of purity - and perhaps as a way for me to explore my own thinking about how code should be reasoned about - here is a brief attempt at code reviewing the mug; in future posts I hope to refactor it...

For my own purposes, lets see how mug.php might look:

$coffee = new Coffee();

if ($coffee->Empty) {
    $coffee->Refill();
} else {
    $coffee->Drink();
}

//I am a software developer

So... as a coffee-drinking engineer, my first thought on looking at the code was "Hang on, can I not refill my mug and drink it straight away?!" This is counter-intuitive, to say the least; it's pretty clear that the code's intention is to refill the mug when empty, but it's hard to imagine that the writer meant for us to forego drinking the coffee when we do so.

Next up, it should be pointed out that we don't have any idea as to the internals of the Coffee class. So what can we glean from the public API? Well, it's an object - "Well done, Chris, nice one!" - and it has a public property called Empty as well as two public methods Refill and Drink. I suppose we can assume it is what might be termed "self-documenting code".

So, perhaps it looks like this:

class Coffee
{
    public $Empty;

    public function Refill()
    {
        // Do something that involves refilling... something
    }

    public function Drink()
    {
        // Do something that involves drinking... something
    }
}

Okay, that's great, that fits the bill. Weird code style choice though - along with PSR-1 I prefer camelCase for class members such as methods and properties. I'm assuming the author used spaces and not tabs... but that's for another day.

But here's problem number one... Object Orientation is about modelling the challenges we face in terms of conceptual objects, their properties, what actions they can perform and how they interact with other objects.

So with that in mind... When is the last time that coffee did anything by itself? When is the last time coffee was empty? Or when the coffee was responsible for its drinking, or refilling? Sure, I suppose it could just pour itself into a willing mouth, or mug... But it's not exactly best-placed to do that without outside influence!

Also, in the mug.php you can see that we instantiate a new Coffee object immediately before checking whether it's empty. But... we just made the coffee. Why is it empty? How is it empty? Can coffee even be empty? (Answer: yes, Starbucks is totally devoid of content...). So does it start off empty? Then we're always going to be refilling our coffee and never actually drinking it...

The answer must be that something is marking it as either empty or not... Do we have sentient coffee? Is it set in the constructor? If so... what's it set to? Or is there something else instantiated in Coffee::__construct that decides whether the coffee is empty? This would be an example of coupling code to a concrete implementation.

Perhaps the presence of the Refill method indicates that it starts out filled? Maybe Drink empties it. But then as we're starting out with a new Coffee every time, does that mean we never have an Empty state? Therefore, perhaps mug.php should really be:

$coffee = new Coffee();

$coffee->Drink();

//I am a software developer

That certainly suits me! Never-ending coffee - living the developer dream! (Thanks to Mark Baker for inspiring the last paragraph.)

Finally, that comment:

//I am a software developer

Like most inline comments these days, it's totally redundant. It tells us nothing about the code around it, and in some ways we could suggest that it is performing the most common function of comments in code - it lies. If a real software developer made this code, they'd have followed more SOLID approach.

Or perhaps the comment is performing a lesser but still perfectly valid function - it's simultaneously an apology, and a cry for help...

Over the next few blog posts I will attempt to refactor this mug, addressing the points above and possibly making a better mug.