DEV Community

Refactoring If-else statement

Mervin on November 13, 2017

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 pra...
Read full post
Collapse
 
nathanheffley profile image
Nathan Heffley • • 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.

Collapse
 
leightondarkins profile image
Leighton Darkins • • 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..."

Collapse
 
mervinsv profile image
Mervin •

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

Collapse
 
fnh profile image
Fabian Holzer • • 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.

Collapse
 
gabrielweidmann profile image
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.

Collapse
 
mervinsv profile image
Mervin •

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

Collapse
 
craser profile image
Grumpy • • 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.

Collapse
 
nijeesh4all profile image
Nijeesh Joshy •

I go with ternary operator when ever i can