Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

This question already has an answer here:

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.

share|improve this question

marked as duplicate by gnat, BЈовић, GlenH7, Dan Pichelman, Jimmy Hoffa Aug 2 '13 at 14:24

This question has been asked before and already has an answer. If those answers do not fully address your question, please ask a new question.

1  
If one runs FindBugs on a large code base (not mine!), some of the mechanically findable errors stem indeed from these if-statements. However just moving code to their own method might hamper readability too. The code style must fit. –  Joop Eggen Aug 1 '13 at 15:15
    
8  
If any so called experts tell you not to use "if/else" at all then they are exactly the type of expert I would simply ignore. –  Ramhound Aug 1 '13 at 15:54
    
@Ramhound Some of the eminent experts and tools suggest reformatting if/else. Look at my edit. –  deviDave Aug 1 '13 at 18:07

6 Answers 6

up vote 16 down vote accepted

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: enter image description here

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.

share|improve this answer
    
Remember that the cost of a method call can be significant, depending on the language. There's almost always a tradeoff between writing readable code and writing performant code. It probably won't make an appreciable difference, and "premature optimization is the root of all evil", but it could have a significant effect if you threw in a loop structure around this whole thing, or if the code were itself in a recursive pattern. –  KeithS Aug 1 '13 at 18:00
    
functions/methods with cyclomatic coplexity less than 10 are not even worth looking at - unless your code is scoring 95% at sonar, have 100% unit test coverage, etc. But CC over 20 is a red flag - you must refactor that asap. –  c69 Aug 1 '13 at 21:32
    
If you really want bad/unreadable code, follow the cyclomatic complexity rubbish for something like switch(myEnumType). –  Brendan Aug 3 '13 at 16:51

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 that:

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

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

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

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

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 than 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.

Avoid combining blocks which has 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 its own implementation of those methods.

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 code even worse. 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;
share|improve this answer
    
-1 for giving dogs Pedigree. +1 for good explanation. if (+1-1==0)==true do nothing; –  Ingo Aug 2 '13 at 11:51
    
#4 is actually pretty fundamental: you can always refactor all conditionals to polymorphism. Smalltalk is the existence proof of that principle, it doesn't even have any conditional construct, there is no if, no else, no switch, no case not even loops! Only virtual method dispatch (called messaging in Smalltalk). Booleans are actually implemented using the same Church Encoding that is typically used to encode Booleans in Lambda Calculus. –  Jörg W Mittag Aug 3 '13 at 0:38

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.

share|improve this answer
1  
+1 for "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". So true. –  Graham Aug 1 '13 at 17:08

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.

share|improve this answer
    
I read exact the same samples on the net :). Not exact like yours, but they explained it in the same way. –  deviDave Aug 1 '13 at 18:09
    
Your first example has a DoYOnly(); case which the latter examples are missing. –  Dan D. Aug 1 '13 at 18:50
    
That first example is tangential; it doesn't match the format of the OP's snippet and was meant to prove a point about how you could, given conditions that could be independently true, lose your place in a nested codebase and think you're somewhere you're not. Edited to clarify –  KeithS Aug 1 '13 at 20:59

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.

share|improve this answer
    
So you just put the negated condition into the else if? That doesn't seem too DRY to me. And there is also the risk that if you change the condition you forget to change the negated one. I do not like it. –  Honza Brabec Aug 1 '13 at 20:00

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.

share|improve this answer
    
good example with car ;) –  deviDave Aug 1 '13 at 18:10
    
In your second example I'd prefer to see those ifs replaced with else-ifs to make it clear that the light states are mutually exclusive. Also, what about a default condition? What if there's a power cut or the lights are broken? Else-ifs would make it easy to add a default else clause. –  Rich Smith Aug 1 '13 at 20:45
    
@RichSmith yes, I agree, but the point of my answer wasn't the code I wrote. It was to illustrate that the else in a if condition includes everything else. The scope of if is narrow but the scope of else is wide. –  Mathew Foscarini Aug 2 '13 at 14:07

Not the answer you're looking for? Browse other questions tagged or ask your own question.