Readable If/Then Statements

A major passion of mine is readable code. When the code is easy to read, it is easy to maintain. It is easy to test. In a lot of the projects we work on, the code we write ends up living for over ten years.

One area I've noticed developers have a nasty habit of making code harder to read is the simple If/Then statement. It is interesting how something that small can be written in so many different ways. A lot of times the developer only what exactly a complex if/then does at the time of writing it. After they leave it for a day they have no idea what it does.

Let's consider a simple validation example.

function validateSomeJsonObject(someJsonObject){
    var isValid = false;

    if (someJsonObject
      && someJsonObject.myValue
      && ((someJsonObject.myValue > 0 && someJsonObject.myValue < 100)
            || someJsonObject.myValue > 1000))
    {
      isValid = true;
    }        


    return isValid;
}

It is hard to know what the rules are just by glancing at the code. By the way, here are the rules I wrote down before writing that if/then statement.

Rules:

1. The object must not be null.
2. The myValue property must not be null.
3. The myValue property must be greater than zero, and less than 100 or the myValue property must be greater than 1000.

Also, unless you did TDD and wrote the test cases first, how can you be sure all the rules were followed. Even if you used a tool like NCrunch for C# or WallabyJS, which highlights the lines of code covered by tests, there is no guarantee tests are covering all the possible permutations of the code.

One of the more common refactors I've seen is breaking out the if/then statement into something I like to call the If/Then statement of doom. I call it that because the If/Then statements stack on each other so much they start looking like a pyramid.

function validateSomeJsonObject(someJsonObject){
    var isValid = false;

    if (someJsonObject) {
        if (someJsonObject.myValue) {
            if (someJsonObject.myValue > 0 && someJsonObject.myValue < 100) {
               isValid = true;
            }                
            else if (someJsonObject.myValue > 1000) {
               isValid = true;            
            }
        }
    }

    return isValid;
}

I've seen many instances where that was the more common refactor. I saw one come through on a recent pull request. When I asked the developer why he elected to refactor it this way he stated these reasons.

1. This developer subscribed to the belief there should be a single point of exit for a method and that exit point should be at the bottom.
2. While stepping through the code, he couldn't tell what part of the if/then statement failed. Breaking it up made it a lot easier to debug.

I don't believe it is that much easier to read. Here is how I would refactor or write a method such as this.

function validateSomeJsonObject(someJsonObject){
    if (!someJsonObject) {
        return false;
    }

    if (!someJsonObject.myValue){
        return false;
    }

    if (someJsonObject.myValue > 0 && someJsonObject.myValue < 100){
        return true;
    }

    if (someJsonObject.myValue > 1000){
        return true;
    }

    return false;    
}

This is much easier to read even though it is six lines longer than the original method. It is also much easier to write tests for. When using a tool such as NCrunch or WallabyJS, it is very easy to see what code is missing unit tests. If a new rule comes along, say a value can also be between 300 and 350 it is very easy to add that to this method.

Finally, in this particular case, I disagree with the single exit point coding philosophy. The method is only 18 lines of code. No scrolling is required to view the entire method. I think the single exit point came about from the days of long methods. It is very confusing when there is a random return statement on a method hundreds of lines long. The first two if/then statements establish an implied contract with the reader of "I'm checking for valid values, the first time I come across something invalid I'm getting out of this method." I believe it is okay to have multiple exit points as long as that contract is honored by the rest of the method.

At first, I wasn't a fan of this approach. It wasn't until recently I saw the real value in it when I came back to a method written by someone else who used this philosophy. Just by glancing at it I could tell what the rules were and it had the appropriate unit test coverage. I felt confident in making a change to the method.