Coffee Cup Code Critique
The title of this post is Coffee Cup Code Critique but in my day to day, I use terms "coffee mug" and "code review." This was an opportunity to use alliteration that I simply could not refuse.
The featured image of this post is a gift I received from my fiancé. When I first opened it, I chuckled and thought it fit me perfectly. It would be my vessel of caffeination each morning in preparation for the work ahead. I examined the mug for a few seconds and then set it aside without thinking much of it - until the next day.
While working through the logic of some code, I often find myself stroking my beard and staring at any object in my vicinity. In this instance, that object was the latest addition to my workspace, the coffee mug. This time I did a thorough examination of the code printed on the mug.
The Code Review
Some notes before we begin...
There is no indication as to what the programming language printed on the mug is. The title contains "Coffee++", so maybe C++ is intended. I will review the code as if it is Javascript, because it is the language I am most comfortable with.
References to line numbers will be to the most recent code checkpoint, not necessarily the original code.
The mug only gives us a snippet of code. We have to make many inferences of what the surrounding code is.
Ignoring the whitespace between coffee and the methods invoked on it, this is what is printed on the mug: { coffee.drink(); work.execute(); if(coffee == "empty") { if(coffeepot == "empty") coffeepot.brew(); coffee.refill(); } }
The first and easiest critique is related to code style. While I am not a fan of adding curly braces on a new line to open a code block, it is certainly valid. The problem is the inconsistent usage of curly braces. The loop on line 1 and conditional on line 5 both open a block with a curly brace, but the conditional on line 7 does not. In a real world scenario, linters should catch this before it reaches the code reviewer. { coffee.drink(); work.execute(); if(coffee == "empty") { if(coffeepot == "empty") { coffeepot.brew(); } coffee.refill(); } }
The second issue is the way the programmer mixed Object Oriented Programming (OOP) and imperative paradigms. For example, the coffee variable on line 3 seems to be an object with a method to drink. This method presumably updates the coffee object's internal state representing the amount of coffee left. But then on line 5, the programmer does a comparison operation, checking if coffee is equal to a string "empty".
One problem is that coffee has type "object" and "empty" has type "string", which will never result in true when compared. Ignoring this fact, the programmer is imperatively comparing these two values. A better approach would be to replace line 5 with something like:
Assuming the isEmpty method is implemented correctly, the code is now adhering to the OOP paradigm, encapsulating state and behavior to the object.
The coffeepot object has the same exact issue. On line 7 it is imperatively comparing values, and on line 9 it is invoking a brew method. { coffee.drink(); work.execute(); if(coffee.isEmpty()) { if(coffeepot.isEmpty()) { coffeepot.brew(); } coffee.refill(); } }
We can continue refactoring to update the variable the loop is checking before each iteration, working. This variable is accessible in this lexical scope, although we are not sure where it is defined. It is not apparent that we will ever break out of this loop based on the snippet available to us. We have to assume that the work.execute() method will eventually set working to some falsy value, breaking the loop. Even if this assumption is correct, this is still bad because it is breaking the encapsulation rule.
A better approach would keep the state of working with the work object. { coffee.drink(); work.execute(); if(coffee.isEmpty()) { if(coffeepot.isEmpty()) { coffeepot.brew(); } coffee.refill(); } }
Wrap Up
There are actually more critiques to be made of that simple code snippet. It is probably a good idea to make some of the methods asynchronous. I'm sure employers would not appreciate if everyone stared at the coffeepot until the brewing completed, instead of doing other tasks in the meantime.
Message to the Seller
As of time of publishing, one of the most "Helpful" reviews of this product on Amazon is not of the mug's build quality, but of the code quality...
To someone who knows c++ that morning conversation will be: where are your constructors and why are you comparing the string "empty" twice when an empty method would be more oop?
One can only assume that the poor quality of your code is negatively impacting your bottom line.