Redundant ElseIf-Else Blocks [duplicate]

Question

This question already has answers here:

Elegant ways to handle if(if else) else (24 answers)

Closed 5 years ago.

These types of if-elseif-else blocks appear all over the place, and in no small number (so the less the better). Every time I have to think and decide: Do I want the simpler or the more thorough of the two... and I don't want to think about it anymore.

For example, if I am expecting only one of three integers (0, 1, 2)... don't I want to know (throw or terminate) if I get something else? (In the case of Example #2, the function do_two() could be triggered by any value other than 0 or 1). Obviously, this may depend on if I want my code to break or to continue upon encountering an anomaly, which rigorous testing should detect anyways (but who can ever be sure?). But I am looking for an answer that is appropriate in all, or most, cases. Please explain your answer, if your reasoning differs from my own.

NOTE REGARDING DUPLICATE: I don't think this is a duplicate for two reasons: The answers to the question suggest using switch statements and code abstraction. I don't use switch statements for something this small due to the overhead, nevermind not liking them much at all. Second, if the code can be abstracted, I may still vary well need to run these checks... plus more overhead. My question is aimed at the necessity in covering corner cases using the final else. Most people, in my experience (which is minimal), would jump to using Example #1 if SCRUMing. I think the best point (comment -> answer?) is yes there is a difference and it depends on whether or not I am coding defensively.

Example #1:

if(!is_null(variable) && is_integer(variable))
{
    if(variable == 0)
        do_zero();
    elseif(variable == 1)
        do_one();
    elseif(variable == 2)
        do_two();
    else
        throw_error();
}

Example #2:

if(!is_null(variable) && is_integer(variable))
{
    if(variable == 0)
        do_zero();
    elseif(variable == 1)
        do_one();
    else
        do_two();
}

Answer 1

Ask yourself, what is the expected behavior of your method when the variable is outside the range? Should it:

• Silently discard it and do nothing? Then your second approach is fine.

• Inform the caller that the variable is wrong? My answer focuses on this case.

Although I would prefer the first approach, it can be improved. The second approach, on the other way, should be avoided, because it makes debugging difficult, since nothing happens when the value is outside the allowed range.

If the variable is outside the range, you may want to know this early in order to terminate (for example by throwing an exception) early too.

For example, this Python code:

def demo(something):
    if something == 0:
        do_zero()
    elif something == 1:
        do_one()
    elif something == 2:
        do_two()
    else:
        raise ValueError("Something is out of range.")

can be rewritten this way:

def demo(something):
    if something < 0 or something > 2:
        raise ValueError("Something is out of range.")

    if something == 0:
        do_zero()
    elif something == 1:
        do_one()
    elif something == 2:
        do_two()

The if block at the beginning of the method is called the guard clause, a refactoring technique used to remove nested conditionals. Here, we don't have nested conditionals, but it is still useful to move arguments validation at the beginning of the method.

Depending on your language, there may be more to do. Most obvious in some languages, such as Python, is the replacement of if/elif/else by a map. The previous method becomes:

def demo(something):
    if something < 0 or something > 2:
        raise ValueError("Something is out of range.")

    # Creating the map between the values of `something` and the corresponding methods to call.
    map = {
        0: do_zero,
        1: do_one,
        2: do_two,
    }

    # Invoking the method.
    map[something]()

From there, you may want to remove the guard clause to obtain a more compact piece of code:

def demo(something):
    map = {
        0: do_zero,
        1: do_one,
        2: do_two,
    }

    if something in map:
        map[something]()
    else:
        raise ValueError("Something is out of range.")

A positive side effect is also the removal of code duplication: you don't have to specify the allowed range separately from the values, which can be the cause of a mistake where you change the list of values but forget to change the corresponding range.

---

In order to reduce the code even further, you may rely on the framework. In Python, for instance, accessing a value in a dictionary with a key which doesn't exist throws an exception, which means that this piece of code:

def demo(something):
    {
        0: do_zero,
        1: do_one,
        2: do_two,
    }[something]()

does the same thing, except that the type of exception is different (KeyError instead of ValueError). Given that the method is small and it's easy to understand what's going here when receiving the KeyError, this last refactoring is acceptable. On the other hand, if you're dealing with a larger method or the types of exceptions are more distant, avoid relying on the framework and throw the exception explicitly (and preferably early).