Mervin

Posted on Nov 13, 2017

Refactoring If-else statement

#discuss #coding #learning

Hi everyone! I just want your opinions/thoughts on how to use if-else statements to make it more readable and maintainable or what are the best practices on using if-else statements. So let's start!

First

Nested if statements

if(!isEmpty(object))
{
    if(IsString(object.number))
        log("Is a String"); 
    else if(IsNumber(object.number))
        log("Is a number");
    else
    log("Is a special character");      
}

or you can have it like this:

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

Which do you think is better? Which one is easy to maintain and read? And what will happen if these IF statements get bigger?

Second

Ternary operator vs if-else-if statements

var tempVar = 0;
if(number == 1)
    tempVar = 1;
else if(number == 2)
    tempvar = 2;
else
    tempvar = 3;

or this one

var tempVar = ((number == 1)? 1: (number == 2)? 2:3);

Which one is better if we are talking about maintainability?

Top comments (8)

Fabian Holzer • Nov 13 '17 • Edited

I personally would use the ternary operator over an if statement when I had a structure like this

if (condition) {
  return firstExpression;
} else {
  return secondExpression;
}

in which the equivalent to condition, firstExpression and secondExpression are rather simplistic expressions, e.g. constants, already calculated values or the evaluation of a function which is free from any side effects.

If I cannot refactor all of those expressions in a readable manner so that the resulting one-liner

return condition ? firstExpression : secondExpression;

does not exceed a certain column width, I would go for the if statement.

Also, if I would advise to abstain from nesting a ternary statement into one or both of the expressions. Even if hidden within a function, I would prefer to have the flow of control visible.

When there is really complex logic, it does not go away by obscurity.

Also, it would not be the first time, that I encountered, in which a lots of complex boolean logic is actually a simple polymorphic dispatch trying to come out of hiding.

Nathan Heffley • Nov 13 '17 • Edited

For the first one, I would write it like this (if returning was an option)

if (isEmpty(object)) {
    log("Is a special character");
    return;
}
if (IsString(object.number))
{
    log("Is  a String");    
}
if (IsNumber(object.number))
{
    log("Is a number");
}

If you stated having more than just the check for a string and a number, I would change this a switch statement (leave the first if that checks if it's empty, and make the rest a switch)

For the second one, I would use a switch statement.

Leighton Darkins • Nov 13 '17 • Edited

My thoughts line up with this, for the most part.

"...and what will happen if these IF statements get bigger?"

One of the best pieces of feedback I got early in my career was to extract boolean expressions into functions when they become too large (typically more than one || or &&)

So if i started with:

if (!isEmpty(object) && isNumber(object.value) && isGreaterThanOne(object.value) {
    log("It's a number that's greater than 1");
}

I'd just snatch that whole boolean expression and extract it into something like:

if (valueIsANumberGreaterThanOne(object)) {
    log("It's a number that's greater than 1");
}

function valueIsANumberGreaterThanOne(object) {
    if (isEmpty(object)) return false;
    if (!isNumber(object.value)) return false;
    return isGreaterThanOne(object.value);
}

This way the if statement is easily human readable.

In the first instance you read "If the object is empty, and the objects' value is a number and the objects' value is greater than one, then..."

In the second instance you read "If the object value is a number that's greater than one, then..."

Mervin • Nov 13 '17

Yeah. Boolean functions are great. It makes your code more readable and usable. And also it removes duplicates.

Gabriel Weidmann • Nov 13 '17

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 • Nov 13 '17

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

Chris Raser • Nov 13 '17 • Edited

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.