Discussion on: Refactoring If-else statement

[Gabriel Weidmann]

To First: Definetly the first one, because think about this case:

var bool = !isEmpty(object);
if(bool && IsString(object.number))
{
    log("Is  a String");    
}
else if(IsNumber(object.number)) //"bool" isn't relevant here
{
    log("Is a number");
}
else if(bool)
{
    log("Is a special character");
}

You could easily miss that bool isn't relevant in the first else-if statement.

To second: I would only take the ternary operator if it's just an replacement for a single if-else. I find nested ternary operators really hard to read and understand.

[Mervin]

Oh sorry. The second if should have a bool condition.

[Chris Raser]

That missing bool is exactly the reason to use a short-circuit return at the top (like in Nathan's note above), or just leaving the enclosing if statement as it is in your original code.

You hit the nail on the head when you asked about what happens when these if statements get bigger.

That growth happens in stages. First, you generally do what Leighton suggests, and refactor the boolean checks into separate functions. At some point that starts to get unwieldy, and you refactor the chain of if/if-else statements into a chain of objects. (See the Chain of Responsibility pattern)

Judging when to do that refactoring comes with time and experience. I personally do it before the current arrangement is causing trouble. (I tend to be an "ounce of prevention" kind of guy.) Two things make that refactoring much easier:

• Being familiar with patterns and having a clear idea of what Pattern you want to refactor into.
• Having solid automated testing in place so that you can be confident that the refactoring hasn't broken anything.

Here's an example of how I might refactor your above if/else chain into something like the Chain of Responsibility pattern:

function NonEmptyScreen(chain) {
    this.accept = function(object) {
        if (!isEmpty(object)) {
            chain.accept(object);
        }
        else {
            console.log("ERROR: Empty object!");
        }
    };        
}

function StringValidator(chain) {
    this.accept = function(object) {
        if (isString(object)) {
            console.log("Is a String")
        }
        else {
            chain.accept(object);
        }
    };
}

function NumberValidator(chain) {
    this.accept = function(object) {
        if (isNumber(object)) {
            console.log("Is a number");
        }
        else {
            chain.accept(object);
        }
    };
}

function DefaultValidator() {
    this.accept = function(object) {
        console.log("Is a special character");
    };
}

function ObjectValidator() {
    this.chain = buildValidationChain();
    this.accept = function(object) {
        this.chain.accept(object);
    };

    function buildValidationChain() {
        var chain = new DefaultValidator();
        chain = new NumberValidator(chain);
        chain = new StringValidator(chain);
        chain = new NonEmptyScreen(chain);
        return chain;            
    }
}

// Helper functions, just added so the above will run.
function isEmpty(o) {
    return o == null;
}

function isString(o) {
    return (typeof o) === "string";
}

function isNumber(o) {
    return (typeof o) === "number";
}

//Quick and dirty testing.
(function test() {
    var inputs = [
        null,
        "I'm a String!",
        42,
        {}
    ];
    var v = new ObjectValidator();

    for (var i in inputs) {
        var input = inputs[i];
        console.log("input: " + input);
        v.accept(input);
    }    

}());

That code is horrific overkill for the statement you started with. But I hope it illustrates the pattern, and gives you some idea of how the code can be refactored as it grows and becomes more complex.