I started a small … umm… argument via twitter (imagine that) about code readability a while back. It all started with this tweet:
— Derick Bailey (@derickbailey)
Shortly after, I received this response:
— Bobby Johnson (@NotMyself)
For clarification, the code in question is:
My basic response was “yes”, but I did mention that there were a number of problems in this… and that I’m also guilty of writing this same code… because, of course I am! How could I possibly complain about someone else’ code if I haven’t written as bad or worse?! :)
My one-tweet rant that started the “conversation” was all about readability and not reducing keystrokes for the sake of a compiler. Rather, we should write for the sake of humans reading and understanding the code… because that’s one of the primary goals of code, in my experience.
From the perspective of readability, this code fails. Completely. I know this because I have written it before and gone off on a “what the hell was I thinking?!” tirade against myself when coming back to this kind of code.
From a readability standpoint, this code fails for a number of reasons, each of which deserves it’s own discussion. There is at least one other, non-readability problem with this code as well. All told, here are my core complaints:
- Assumption of arcane syntax knowledge
- Shortcut if-then logic, requiring knowledge of order of precedence
- Code will fail is “change” is not a function
- Possible violation of Liskov Substitution Principle
Coercion, Arcane Syntax && Short-cut Logic
The code in question is a short-hand form of the following logic:
If the “options” object has an attributed “change”, execute “options.change” as a method
The first half of the code either returns undefined or the contents of the “change” attribute. The second half executes “options.change()” as a method. It’s the && that makes all the difference, though.
It would be better to write code like this:
Any of these variations does approximately the same thing, though they do get more specific about where the “change” attribute is located. But even if this code is more clearly stating the intended logic, it still has problems because it will fail for anything other than a function in “change”.
Failing If “change” Is Not A Function
Say you have an object that looks like this:
What do you think will happen when the “options.change && options.change()” code is called? It won’t be pretty, that’s for sure:
The code that checks for the existence of “change” assumes the content of that attribute is a function… which is not guaranteed to be true. To better handle this situation, you should check to see if it’s a function. The easiest way to do that is to use underscore or lodash:
Now the code is looking better – checking to see if the “change” attribute is a function or not, and only executing the function if it is. This code is more verbose, yes, but it is more expressive. The implicit check for the existence of the attribute has been replaced with an explicit check to see if the attribute exists as a function.
Replacing implicit with explicit is almost always a good move, in my experience. Yes, there are exceptions to this. No, I won’t be detailing the exceptions at this point. It’s never a simple, cut and dry, black and white.
Even if this code is more explicit and understandable, there is still one potential problem with it.
Violation Of Liskov Substitution Principle
The Liskov Substitution Principle (PDF file link) says:
Functions that use pointers or references to base classes must be able to use objects of derived classes without knowing it.
This is a rather fancy way of saying that you should conform to an API, both in the technical sense and in the semantic sense. You can’t make a “change” method create something new, for example. That would break the semantics of “change”. You also can’t have your “change” method take different parameters than the original API definition of “change”. That would break the API contract. If you want to provide your own “change” method, you have to make it conform to both the API contract (method signature / object shape) and the semantics.
The code in question, above, is a potential violation of this object substitution principle because it provides opportunity – expectation – that your object will break the API contract.
In my experience (having been the developer who created Marionette.js – a library that is chock-full of this method checking code), it is better to split the API definitions in to multiple parts. So, instead of having one object definition that can have optional methods and attributes, you provide documentation for multiple object definitions – each of which serve a specific purpose. In this way, you can skip the if-method-exists checks. You can assume the method is available if the object is available.
For example, if the above code were changed to “options.save && options.save()”, then the API definitions could be split up in to an “options” data structure and a “persistence” object that knows how to save the options data structure:
This code could still be called “bad” – you might want the save method call to happen somewhere else, so you don’t mix up the state change with state persistence. But that’s another discussion for a specific scenario…
It’s Not Always That Simple
There’s never any real black and white situations. There are times when it’s ok to write short-hand syntax. There are times when it’s ok to have optional methods and attributes. There are times when just about everything that you or I think is wrong with code, is acceptable and may be the better option.
The real key is to stop assuming based on incomplete knowledge or “that’s how I’ve always done” mentalities. Explore the options. Look at whether or not your current situation calls for this short-hand, implicit coding style. Chances are, your code will be more robust and fault-tolerant if you code with a more explicit form.