Our Fathers’ faults.
Intelligent people learn from their mistakes, wise people learn from other’s mistake. Unlucky people learn from the mess they have to fix in someone else’s code.
Working as a programmer is not always an easy task. On lucky days you feel like a god planning and designing architectures, wisely crafting virtual nuts, cogs, and bolts that happily tick together in an elegant and coordinated choreography, while on bad days it is bug fixing in your code. In really sh**ty days it is bug hunting in code written (by a long-gone someone else) for exploration, demo, and test of the coolness factor.
Having inherited a large codebase full of… well, funny stuff, I too often find myself in the latter category. So I am not that reluctant in exposing all the mistakes, errors, misconceptions and wrongdoings I found during the last five years. After all, for most of them, even if this is my codebase, it is not really my fault.
I’ll tackle the matter from an engineering point of view, with the goal of team-writing reliable and maintainable code. Maybe some people will find this goal has an insufficient coolness factor. Feel free, but, in that case, take care not to leave your code on my path :-).
The idea has to be credited to my colleague Roberto who suggested this as a topic for a meetup talk. Maybe I am not yet that prepared for such an event, so I’ll start here with some posts.
Failure is not an Option
Let’s start from Option[T]. I won’t go into “What is an Option[T]” road, but if you speak C++ you may want to lookup std::optional.
This generic (template) seems such a great way to get rid of those pesky null pointers and those disturbing special negative integers or empty strings and move the optionality idea right into the type.
If a value might not exist then the Option[T] is the right container for it, isn’t it? What can be wrong?
Well, for every good idea, there is at least a wrong way to apply it. Let’s just dive into the example code:
for {
position ← packet.parsePosition
duration ← packet.parseDuration
color ← packet.parseColor
}
yield
publish( Command( position, duration, color ) )
The intent of the code is to parse a command frame (packet) which should contain a position, a duration, and a color. These are optional since the frame is likely to be from a JSON document (or a protocol buffer, anyway neither format has a way to mandate fields). So, if you shell out the options, in the happy path you will get an action (publish) and a result that will be Some of whatever type is returned by publish function.
But what happens when something does not work as expected? Let’s say packet.parseDuration returns None. Then the for comprehension, shortcuts to None without invoking publish (correct), but without leaving a clue for whatever went wrong.
The fail is silent.
Silent failure in case of malformed input data is a bad idea for real-world applications, living in a likely hostile environment. At least at a logging level, you should know why your program hasn’t properly reacted.
Note that you don’t need an exception, you may go along with log lines such in the code below that is a refactoring of the aforementioned example.
val decoded : Either[String,Command] = for {
position ← packet.parsePosition.toRight( “’position’ field missing” )
duration ← packet.parseDuration.toRight( “’duration’ field missing” )
color = packet.parseColor.toRight( "'color' field missing" )
}
yield
Command( position, duration, color )
decoded.fold( log.error, publish )
Lesson learned – if a value might not exist for a legit reason, that is it is actually an option, then Option[T] is the right container. It the optionality is the result of a possible error stemming from input processing or environment condition, then Either[L,R] (std::variant in C++) is a better choice.