Clarification of “avoid if-else” advice [duplicate]

Question

This question already has answers here:

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

Closed 7 years ago.

The experts in clean code advise not to use if/else since it's creating an unreadable code. They suggest rather using IF and not to wait till the end of a method without real need.

Now, this if/else advice confuses me. Do they say that I should not use if/else at all (!) or to avoid if/else nesting?

Also, if they refer to the nesting of if/else, should I not do even a single nesting or I should limit it to max 2 nestings (as some recommends)?

When I say single nesting, I mean this:

    if (...) {

        if (...) {

        } else {

        }

    } else {

    }

EDIT

Also tools like Resharper will suggest reformatting if/else statements. It usually concerts them to if stand-alone statement, and sometimes even to ternary expression.

Answer 1

I think this advice came from a software metric which called Cyclomatic complexity or Conditional complexity check this wiki page

Definition:

The cyclomatic complexity of a section of source code is the count of the number of linearly independent paths through the source code. For instance, if the source code contained no decision points such as IF statements or FOR loops, the complexity would be 1, since there is only a single path through the code. If the code had a single IF statement containing a single condition there would be two paths through the code, one path where the IF statement is evaluated as TRUE and one path where the IF statement is evaluated as FALSE.

So

Do they say that I should not use if/else at all (!) or to avoid if/else nesting?

No you shouldn't avoid if/else at all! but you should be careful about Cyclomatic complexity of your code. More code complexity tends to have more defects. According to this the right limit would be 11. A tool called Code Complete categorize the score like that Ref.:

• 0-5 - the routine is probably fine
• 6-10 - start to think about ways to simplify the routine
• 10+ - break part of the routine into a second routine and call it from the first routine

How to calculate Cyclomatic complexity "basically":

From example here: consider the following pseudo code :

if (A=354) {

    if (B>C) {/*decision 1*/ A=B} else {/*decision 2*/ A=C}

   }/*decision 3 , in case A is NOT equal 354*/ 
print A

please note that this metric is concerned about number of decision in your program/code so from the code above we could say that we have 3 decisions ( 3 values of A)

let's calculate it formally so consider the following control flow of the code:

The complexity M is then defined* as:Ref

M = E − N + 2P

where

E = the number of edges of the graph
N = the number of nodes of the graph
P = the number of connected components (exit nodes).

by applying the formula E(flow lines) = 8 , N (nodes)= 7 , P (exit nodes) =1 then M = 8-7 + (2*1) = 3

*there is also another definition stated but it is close :M = E − N + P check the reference for theory.

Please note that many of static code analysis tools could calculate the Cyclomatic complexity of your code.

Answer 2

IMO, the previous answers focus on edge cases and code which should be rewritten anyway. Here are some cases I see nearly everywhere in which if/else should have been replaced by if or another language construct.

Note: I make the answer community wiki. Feel free to modify it and add other examples of cases where if/else usually leads to bad code.

Beginners, ye be warned! if/else has a potential of creating an unreadable code when not used carefully. The next examples show the most common cases where if/else is easily misused.

1. Guard clause

I stopped counting how many times I've seen something like this:

void Demo(...)
{
    if (ok_to_continue)
    {
        ... // 20 lines here.
    }
    else
    {
        throw new SomeException();
    }
}

Nobody needs extra indentation. The same code could have been written like this:

void Demo(...)
{
    if (!ok_to_continue)
    {
        throw new SomeException();
    }

    ... // No indentation for the next 20 lines.
}

Rule: if you can remove an else statement by inverting the if check, handling an error, and exiting immediately, (else otherwise continue on), then do so.

2. Code duplication

This one is also quite frequent.

if (!this.useAlternatePanel)
{
    currentInput = this.defaultTextBox.Text;
    bool missingInput = currentInput.Length == 0;
    this.defaultProcessButton.Enabled = !missingInput;
    this.defaultMissingInputHelpLabel.Visible = missingInput;
}
else
{
    currentInput = this.alternateTextBox.Text;
    bool missingInput = currentInput.Length == 0;
    this.alternateProcessButton.Enabled = !missingInput;
    this.alternateMissingInputHelpLabel.Visible = missingInput;
}

This is an excellent example of beginners' code, because a beginner wouldn't always see how such code can be refactored. One of the ways is to create a map of controls:

var defaultControls = new CountriesProcessingControls(
    this.defaultTextBox,
    this.defaultProcessButton,
    this.defaultMissingInputHelpLabel);

var alternateControls = new CountriesProcessingControls(
    this.alternateTextBox,
    this.alternateProcessButton,
    this.alternateMissingInputHelpLabel);

void RefreshCountriesControls(CountriesProcessingControls controls)
{
    currentInput = controls.textBox.Text;
    bool missingInput = currentInput.Length == 0;
    controls.processButton.Enabled = !missingInput;
    controls.missingInputHelpLabel.Visible = missingInput;
}

RefreshCountriesControls(this.useAlternatePanel ? alternateControls : defaultControls);

Rule: if the block in else is nearly identical to the block in if, you're doing it wrong and causing code duplication.

3. Trying to do two things

The opposite is also common:

if (...)
{
    foreach (var dateInput = this.selectedDates)
    {
        var isConvertible = convertToDate(dateInput);
        if (!isConvertible)
        {
            this.invalidDates.add(dateInput);
        }
    }
}
else
{
    var languageService = this.initializeLanguageService();
    this.Translated = languageService.translate(this.LastInput);
    if (this.Translated != null)
    {
        this.NotificationScheduler.append(LAST_INPUT_TRANSLATED);
    }
}

The block within if has nothing to do with the block within else. Combining two blocks in the same method makes the logic difficult to understand and error-prone. Finding a name for the method, as well as commenting it properly is extremely difficult too.

Rule: Avoid combining blocks which have nothing in common together in an if/else block.

4. Object-oriented programming

This sort of coding is also frequent among beginners:

// `animal` is either a dog or a cat.
if (animal.IsDog)
{
    animal.EatPedigree();
    animal.Bark();
}
else // The animal is a cat.
{
    animal.EatWhiskas();
    animal.Meow();
}

In object-oriented programming, this code shouldn't exist. It should be written like this instead:

IAnimal animal = ...;
animal.Eat();
animal.MakeSound();

given that each class (Cat : IAnimal and Dog : IAnimal) will have their own implementation of those methods.

Rule: when doing object-oriented programming, generalize functions (methods) and use inheritance and polymorphism.

5. Assigning a value

I see this one everywhere in codebases written by beginners:

int price;
if (this.isDiscount)
{
    price = -discount;
}
else
{
    price = unitCost * quantity;
}

This is a terrible piece of code, since it's not written for humans.

• It is misleading. It makes you think that the main operation here is the choosing between discount and non-discount mode, while the actual main operation is the assignment of price.

• It is error prone. What if later, one condition is changed? What if one of two assignments is removed?

• It invites even worse code. I've seen a lot of pieces like that where int price was instead int price = 0;, just to get rid of compiler warning if one of the assignments is missing. Hiding warnings instead of solving them is one of the worst things one can do.

• There is a tiny code duplication, which means more characters to type originally, and more changes to do later.

• It's uselessly long.

The same could be written like this:

int price = this.isDiscount ? -discount : unitCost * quantity;

Rule: use the ternary operator instead of if/else blocks for simple value-assignments.

Answer 3

I really like what Abdurahman said in his answer.

Another way to look at the “avoiding if/else” statement is to think in terms of decisions.

The objective is to avoid confusion when following code. When you have multiple if/else statements that are nested, it becomes hard to understand the functions main objective. The programmer writing the code understands the logic now. BUT, when that same programmer revisits that code a year or two down the road, the understanding is generally forgotten. A new programmer looking at the code has to be able to identify what is going on. So the objective is to break complexity up into simple, easy to understand functions. The objective is not to avoid decisions, but to establish an easy to understand flow of logic.

Answer 4

Code block nesting, aka the "arrowhead" anti-pattern, reduces readability of code because you must either continue shifting to the right, reducing the available width for lines of "real" code on the screen, or you must violate the indenting and other formatting rules that in the normal case make code easier to read.

Case in point:

if (X) {
    if (Y) {
       DoXandY();
    } else {
       DoXOnly();
    }
} else {
   DoDefault();
}

This is a simple example and I wouldn't have issues with a few levels of this type of nesting, provided that the total LOC for this structure was only about a screen long, and that the formatting involved didn't add too much whitespace on the left hand side (consider using 3 or 4 spaces instead of a tabspace, especially if the code will be viewed in other editors where a tabspace may be 8 spaces or more). Beyond that, you may forget or not easily be able to see the initial condition under which the others are nested. This can cause you to get "lost" in your codebase, especially if you have a structure where X and Y are independent:

if (X) {
    if (Y) {
       DoXandY();
    } else {
       DoXOnly();
    }
} else {
   if (Y) {
       DoYOnly();
    } else {
       DoDefault();
    }
}

In this alternate universe to the OP's specific question, now we have two "if(Y)" condition checks, and if, in a real piece of code, we couldn't see the nested parent condition, we could think we're in a different place in code than we really are, and that causes bugs.

One way to reduce nesting is to distribute the outer condition into the inner conditions. Back to your initial snippet (no "DoYOnly()"), consider the following:

if (X && Y) {
    DoXandY();
} else if (X) {
    DoXOnly();
} else {
    DoDefault();
}

Look Ma, no nesting! At the cost of an additional evaluation of X (which, if it is computationally complex to evaluate X, you can perform up front and store in a boolean variable), you make the structure a single-level if/else decision tree, and when editing each branch, you know the condition that must be true to enter it. You must still remember what wasn't true in order to get to that "else if", but because everything's at the same level it should be easier to identify the previous branches of the decision tree. Alternately, you can remove all the else keywords by testing a more complete version of the condition at every step:

if (X && Y) {
    DoXandY();
} 
if (X && !Y) {
    DoXOnly();
} 
if(!X) {
    DoDefault();
}

Now you know exactly what must be true for each statement to be executed. However, this starts toeing a line of other readability issues; it becomes harder to see that all of these statements are mutually exclusive (which is part of what an "else" block does for you).

Other ways to include nesting include reversing one or more conditions. If most or all of the code of a function is within an if statement testing a condition, then you can usually save yourself that level of nesting by instead testing that the condition is not true, and performing anything you'd do in an else block beyond the original if statement, before returning out of the function. For instance:

if(!X) {
    DoDefault();
    return;
}

if(Y) {
    DoXAndY();
} else {
    DoXOnly();
}

This behaves the same way as both of the above statements, but only tests each condition once, and has no nested ifs. This pattern is sometimes known as a "guard clause"; the code below the initial if statement will never execute if X is false, so you can say the if statement "guards" the rest of the code (which is why we never need to test that X is true in the rest of the code; if we get to that line, X is true because we would have exited the function early otherwise). The downside is that this doesn't work as well for code that's a smaller part of a larger function. If there were a "DoMoreWork()" function after the entire snippet above, it would have to be included in both the guard clause code and below the remaining conditional statement. Or, the guard clause could be integrated with the remaining code using the "else if" pattern:

if(!X) {
    DoDefault();        
} else if(Y) {
    DoXAndY();
} else {
    DoXOnly();
}

DoMoreWork();

In this example, DoMoreWork() is always executed even if X is false. Adding the same call below the previous snippet with the return statement would cause DoMoreWork to be called only if X is true, but regardless of whether Y is true or false.

Answer 5

In addition to the other answers, the advice may also mean that if/else should be substituted with if/else if where proper. The latter is more informative and readable (excluding trivial cases of course, like if (x > 0) else if (x <= 0)).

if/else if has one more advantage than just being more readable (though that is a considerable advantage too). It helps the programmer avoid mistakes in case when there are more than two branches depending on the condition. Such style will make the missed case more obvious.

Answer 6

The reason they are advising you to only use if statements, and not else is to avoid the classic bug of if this then for everything else do this. The scope of else is difficult to predict.

For example; let's say I am programming the controls for a self-driving car.

if (light is red) then {
    stop car
} else {
    drive forward
}

While the logic is sound. When the car comes to a yellow light it will keep driving thru the intersection. Likely causing an accident when it should have slowed down and stopped.

Without the if/else the logic is less prone to mistakes.

if(light is red) then {
  stop car
}
if(light is yellow) then {
  slow down
}
if(light is green) then {
  drive forward
}

While my code might not make the best self-driving car. It just demonstrates the difference between an ambiguous condition and explicit conditions. Which is what the author was likely referring too. Don't use else to short cut logic.