Saving the world with a code review
This morning I noticed the following tweet by fellow programmer (and runner) Arun Gupta:
Why code reviews are important? pic.twitter.com/8KyMo7Syis— Arun Gupta (@arungupta) August 28, 2016
The tweet contained the following cartoon by ‘Oppressive Silence’, check out their website for more laughs!
Solved by a code review?
The main question I’d like to ask:
Is this really something that you’ll find during a code review?
I think my answer below won’t surprise you, but the reasoning behind it might. First lets look at four reasons this bug should never happen to me (or any other experienced programmer).
1) Static variables/global state
The first obvious problem with the code is the usage of static state. The variable ‘isCrazyMurderingRobot’ is mutable and static, so anywhere in the code the value can be changed. This makes it very hard to reason about the variable and keep track of it. Any programmer can come along and change the value in some method. This is unacceptable, there is almost no reason to use mutable static variables.
Top tip: If you have variables, especially mutable variables, keep their scope as small as possible!
2) Final method arguments
If you solve the global state problem you’ll probably end up with something like this (translated to Java):
Whenever an argument is passed to a method I have the habit (and my IDE-settings enforce this for me) to make them final. Method arguments should never change inside a method. It is just strange if you need to do this when you think about it. It probably means the method should have its own (scoped) mutable variable.
In the code the following would happen:
The code won’t even compile before telling me what is wrong, the world is saved!
3) Static code analysis
Another reason this bug will never happen in my code is because we have static code analysis on all our projects. A program like FindBugs will immediately flag this as a major problem, assigning values inside an if-statement.
Look at this particular check in FindBugs:
Static code analysis is an essential tool in modern programming, it is just so convenient, never reads over certain bugs like a human would (spoiler: more on that in the final conclusion below).
4) Yoda conditions
The final reason this bug will never happen in my code is: Yoda conditions
A habit I learned a long time ago (in older languages) is writing my if-statements like Yoda. Good it is, mistakes you won’t make. The reason behind doing this is that it’ll prevent exactly the bug in the cartoon.
In the cartoon the if-statement would change from: ‘is crazy murdering robot true?’ to ‘true is crazy murdering robot?’.
If you do this, the following happens:
Conclusion / saving the world
So, do I think Arun is right? Will code reviews find the bug in the cartoon? Yes and no.
Humans are good at reasoning, thinking, being creative, finding bugs, but there is one thing we are horrible at: syntax parsing.
During a code review you are just as likely (maybe even more likely) to read over this typo and don’t notice it. A second pair of eyes won’t save the world here just yet.
But: when reviewing this piece of code, I will complain about the global state! If you start to refactor, encapsulating the logic and remove the mutable static variable the bug will be noticed and solved. So yes, if you do a code review, the code would have be been fixed and humans will be safe for just a little bit longer.