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!

_{(Source of the comic)}

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):

    private void interact_with_humans(boolean isCrazyMurderingRobot) {
        if(isCrazyMurderingRobot = true) {
            kill(humans);
        } else {
            be_nice_to(humans);
        }
    }

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:

    private void interact_with_humans(final boolean isCrazyMurderingRobot) {
        if(isCrazyMurderingRobot = true) {
        // Compilation error: 'Cannot assign a value to final variable 'isCrazyMurderingRobot'

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:

QBA: Method assigns boolean literal in boolean expression (QBA_QUESTIONABLE_BOOLEAN_ASSIGNMENT)

This method assigns a literal boolean value (true or false) to a boolean variable inside an if or while expression. Most probably this was supposed to be a boolean comparison using ==, not an assignment using =.

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:

    private void interact_with_humans(boolean isCrazyMurderingRobot) {
        if(true = isCrazyMurderingRobot) {
        // Compilation error: 'Variable expected'

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.