18

Here is a very simplified example. This isn't necessarily a language-specific question, and I ask that you ignore the many other ways the function can be written, and changes that can be made to it.. Color is of a unique type

string CanLeaveWithoutUmbrella()
{
    if(sky.Color.Equals(Color.Blue))
    {
        return "Yes you can";
    }
    else
    {
        return "No you can't";
    }
}

A lot of people I've met, ReSharper, and this guy (whose comment reminded me I've been looking to ask this for a while) would recommend refactoring the code to remove the else block leaving this:

(I can't recall what the majority have said, I might not have asked this otherwise)

string CanLeaveWithoutUmbrella()
{
    if(sky.Color.Equals(Color.Blue))
    {
        return "Yes you can";
    }
    return "No you can't";
}

Question: Is there an increase in complexity introduced by not including the else block?

I'm under the impression the else more directly states intent, by stating the fact that the code in both blocks is directly related.

Additionally I find that I can prevent subtle mistakes in logic, especially after modifications to code at a later date.

Take this variation of my simplified example (Ignoring the fact the or operator since this is a purposely simplified example):

bool CanLeaveWithoutUmbrella()
{
    if(sky.Color != Color.Blue)
    {
        return false;
    }
    return true;
}

Someone can now add a new if block based on a condition after the first example without immediately correctly recognizing the first condition is placing a constraint on their own condition.

If an else block were present, whoever added the new condition would be forced to go move the contents of the else block (and if somehow they gloss over it heuristics will show the code is unreachable, which it does not in the case of one if constraining another).

Of course there are other ways the specific example should be defined anyways, all of which prevent that situation, but it's just an example.

The length of the example I gave may skew the visual aspect of this, so assume that space taken up to the brackets is relatively insignificant to the rest of the method.

I forgot to mention a case in which I agree with the omission of an else block, and is when using an if block to apply a constraint that must be logically satisfied for all following code, such as a null-check (or any other guards).

15
  • In reality, when there is an "if" statement with a "return", it can be seen as "guard block" in >50% of all cases. So I think your misconception is here that a "guard block" is the exceptional case, and your example the regular case. – Doc Brown Sep 6 '14 at 8:43
  • 2
    @AssortedTrailmix: I agree that a case like the above may be more readable when writing the else clause (to my taste, it will be even more readable when leaving out the unneccessary brackets. But I think the example is not well choosen, because in the case above I would write return sky.Color == Color.Blue (without if/else). An example with a different return type than bool would probably make this clearer. – Doc Brown Sep 6 '14 at 16:03
  • 1
    as pointed in comments above, suggested example is too simplified, inviting speculative answers. As for the part of the question that starts with "Someone can now add a new if block...", it has been asked and answered many times before, see eg Approaches to checking multiple conditions? and multiple questions linked to it. – gnat Sep 6 '14 at 21:05
  • 1
    I fail to see what else blocks have to do with the DRY principle. DRY has more to do with abstraction and references to commonly used code in the form of functions, methods, and objects than what you are asking. Your question is related to code readability and possibly conventions or idioms. – Shashank Gupta Sep 7 '14 at 1:39
  • 2
    Voting to reopen. The good answers to this question are not particularly opinion-based. If the given example in a question is inadequate, improving it by editing it would be the reasonable thing to do, not voting to close for a reason that is not actually true. – Michael Shaw Sep 7 '14 at 5:34

16 Answers 16

27

In my view, the explicit else block is preferable. When I see this:

if (sky.Color != Blue) {
   ...
}  else {
   ...
}

I know that I'm dealing with mutually exclusive options. I don't need to read whats inside the if blocks to be able to tell.

When I see this:

if (sky.Color != Blue) {
   ...
} 
return false;

It looks, on first glance, that it returns false regardless and has an optional side effect the sky isn't blue.

As I see it, the structure of the second code doesn't reflect what the code actually does. That's bad. Instead, choose the first option which reflects the logical structure. I don't want to have to check for return/break/continue logic in each block to know the logical flow.

Why anyone would prefer the other is a mystery to me, hopefully someone will take the opposite position will enlighten me with their answer.

I forgot to mention a case in which I agree with the omission of an else block, and is when using an if block to apply a constraint that must be logically satisfied for all following code, such as a null-check (or any other guards).

I'd say that guard conditions are okay in the case that you've left the happy path. An error or failure has occurred which prevents the ordinary execution from continuing. When this happens you should throw an exception, return an error code, or whatever is appropriate for your language.

Shortly after the original version of this answer, code like this was discovered in my project:

Foobar merge(Foobar left, Foobar right) {
   if(!left.isEmpty()) {
      if(!right.isEmpty()) {
          Foobar merged = // construct merged Foobar
          // missing return merged statement
      }
      return left;
   }
   return right;
}

By not putting the return inside elses, the fact that a return statement was missing was overlooked. Had else's been employed, the code wouldn't even have compiled. (Of course, the far greater concern I have is that the tests written on this code were pretty bad not to detect this issue.)

13
  • That about sums up my thoughts on it, I'm glad I'm not the only one who thinks like this. I work with programmers, tools, and IDEs preferring the removal of the else block (it can be a bothersome when I'm forced to do it just to keep inline with conventions for a codebase). I too am interested to see the counter point here. – Selali Adobor Sep 6 '14 at 2:01
  • 1
    I vary on this. Some times the explicit else isn't gaining you much since it's unlikely to trigger the subtle logic errors the OP mentions - it's just noise. But mostly I agree, and occasionally I will do something like } // else where the else would normally go as a compromise between the two. – Telastyn Sep 6 '14 at 3:08
  • 3
    @Telastyn, but what do you gain from skipping the else? I agree the benefit is very slight in many situations, but even for the slight benefit I think it's worth doing it. Certainly, it seems bizarre to me to put something in a comment when it could be code. – Winston Ewert Sep 6 '14 at 4:29
  • 2
    I think you have the same misception as the OP - "if" with "return" can be mostly seen as a guard block, so you are discussing here the exceptional case, while the more frequent case of guard blocks is IMHO more readable without the "else". – Doc Brown Sep 6 '14 at 8:47
  • ... so what you wrote is not wrong, but IMHO not worth the many upvotes you got. – Doc Brown Sep 6 '14 at 12:16
23

The principal reason for removing the else block that I have found is excess indenting. The short-circuiting of else blocks enables a much flatter code structure.

Many if statements are essentially guards. They're preventing the dereferencing of null pointers and other "don't do this!" errors. And they lead to a quick termination of the current function. For example:

if (obj === NULL) {
    return NULL;
}
// further processing that now can assume obj attributes are set

Now it may seem that an else clause would be very reasonable here:

if (obj === NULL) {
    return NULL;
}
else if (obj.color != Blue) {
   // handle case that object is not blue
}

And indeed, if that's all you're doing, it's not much more indented than the example above. But, this is seldom all you're doing with real, multi-level data structures (a condition that occurs all the time when processing common formats like JSON, HTML, and XML). So if you want to modify the text of all children of a given HTML node, say:

elements = tree.xpath(".//p");
if (elements === NULL) {
    return NULL;
}
p = elements[0]
if ((p.children === NULL) or (p.children.length == 0)) {
    return NULL;
}
for (c in p.children) {
    c.text = c.text.toUpperCase();
}
return p;

The guards do not increase the indentation of the code. With an else clause, all of the actual work starts moving over to the right:

elements = tree.xpath(".//p");
if (elements === NULL) {
    return NULL;
}
else {
    p = elements[0]
    if ((p.children === NULL) or (p.children.length == 0)) {
        return NULL;
    }
    else {
        for (c in p.children) {
            c.text = c.text.toUpperCase();
        }
        return p;
    }
}

This is starting to have significant rightward motion, and this is a pretty trivial example, with only two guard conditions. Every additional guard adds another indentation level. Real code I have written processing XML or consuming detailed JSON feeds can easily stack up 3, 4, or more guard conditions. If you always use the else, you end up indented 4, 5, or 6 levels in. Maybe more. And that definitely contributes to a sense of code complexity, and more time spent understanding what lines up with what. The quick, flat, early-exit style eliminates some of that "excess structure," and seems simpler.

Addendum Comments on this answer made me realize that it might not have been clear that NULL/empty handling is not the only reason for guard conditionals. Not nearly! While this simple example focuses on NULL/empty handling and doesn't contain other reasons, searching deep structures such as XML and ASTs for "relevant" content, for example, often has a long series of specific tests to weed out irrelevant nodes and uninteresting cases. A series of "if this node is not relevant, return" tests will cause the same kind of rightward drift that NULL/empty guards will. And in practice, subsequent relevance tests are frequently coupled with NULL/empty guards for the correspondingly deeper data structures searched--a double whammy for rightward drift.

12
  • 2
    This is a detailed and valid answer, but I'm going to add this to the question (It escaped my mind at first); There is an "exception" where I always find an else block superfluous, when using an if block to apply a constraint that must be logically satisfied for all following code, such as a null-check (or any other guards). – Selali Adobor Sep 6 '14 at 3:08
  • @AssortedTrailmix I believe if you exclude the cases where you can routinely make an early exit in the then clause (such as successive guard statements), then there is no particular value to avoiding the particular else clause. Indeed, it would reduce clarity and potentially be dangerous (by failing to simply state the alternative structure that is/should be there). – Jonathan Eunice Sep 6 '14 at 3:18
  • I think it is reasonable to use guard conditions to eject once you've left the happy path. However, I also find for the case of processing XML/JSON that if you validate input against a schema first, you get better error messages and cleaner code. – Winston Ewert Sep 6 '14 at 4:22
  • This is a perfect explanation of the cases where else is avoided. – Chris Schiffhauer Sep 6 '14 at 6:20
  • 2
    I'd have wrapped the API to not produce silly NULLs instead. – Winston Ewert Sep 6 '14 at 15:34
5

When I see this:

if(something){
    return lamp;
}
return table;

I see a common idiom. A common pattern, which in my head translates to:

if(something){
    return lamp;
}
else return table;

Because this is a very common idiom in programming, the programmer who is used to seeing this kind of code has an implicit 'translation' in their head whenever they see it, that 'converts' it to the proper meaning.

I don't need the explicit else, my head 'puts it there' for me because it recognized the pattern as a whole. I understand the meaning of the entire common pattern, so I don't need small details to clarify it.

For example, read the following text:

enter image description here

Did you read this?

I love Paris in the springtime

If so, you are wrong. Read the text again. What's actually written is

I love Paris in the the springtime

There are two "the" words in the text. So why did you miss one of the two?

It's because your brain saw a common pattern, and immediately translated it to its known meaning. It did not need to read the entire thing detail by detail to figure out the meaning, because it already recognized the pattern upon seeing it. This is why it ignored the extra "the".

Same thing with the above idiom. Programmers who've read a lot of code are used to seeing this pattern, of if(something) {return x;} return y;. They don't need an explciit else to understand it's meaning, because their brain 'puts it there for them'. They recognize it as a pattern with a known meaning: "what's after the closing brace will run only as an implicit else of the previous if".

So in my opinion, the else is unnecessary. But just use what seems more readable.

2

They add visual cluttering in the code, so yes, they might add complexity.

However, the opposite is also true, excessive refactoring to reduce the code length can also add complexity (not in this simple case of course).

I agree with the statement that the else block states intent. But my conclusion is different, because you're adding complexity in your code in order to achieve this.

I disagree with your statement that it allows you to prevent subtle mistakes in logic.

Let's see an example, now it should only return true if the Sky is Blue and there is high Humidity, the Humidity is not high or if the Sky is Red. But then, you mistake one brace and place it in the wrong else:

bool CanLeaveWithoutUmbrella() {
    if(sky.Color == Color.Blue)
    {
        if (sky.Humidity == Humidity.High) 
        {
            return true;
        } 
    else if (sky.Humidity != Humidity.High)
    {
        return true;
    } else if (sky.Color == Color.Red) 
    {
            return true;
    }
    } else 
    {
       return false;
    }
}

We've seen all this kind of silly mistakes in production code and can be very difficult to detect.

I would suggest the following:

I find this easier to read, because I can see at a glance that the default is false.

Also, the idea of forcing to move the contents of a block to insert a new clause, is prone to errors. This somehow relates to the open/closed principle (code should be open for extension but closed for modification). You're forcing to modify the existing code (e.g. the coder might introduce an error in braces balancing).

Finally, you're leading people to answer in a predetermined way that you think is the right one. By instance, you present a very simple example but afterwards, you state that people should not take it into consideration. However, the simplicity of the example affects the whole question:

  • If the case is as simple as the presented one, then my answer would be to omit any if else clauses at all.

    return (sky.Color == Color.Blue);

  • If the case is a bit more complicated, with various clauses, I would answer:

    bool CanLeaveWithoutUmbrella() {

    if(sky.Color == Color.Blue && sky.Humidity == Humidity.High) {
        return true;
    } else if (sky.Humidity != Humidity.High) {
        return true;
    } else if (sky.Color == Color.Red) {
        return true;
    }
    
    return false;
    

    }

  • If the case is complicated and not all clauses return true (maybe they return a double), and I want to introduce some breakpoint or loggin command, I would consider something like:

    double CanLeaveWithoutUmbrella() {
    
        double risk = 0.0;
    
        if(sky.Color == Color.Blue && sky.Humidity == Humidity.High) {
            risk = 1.0;
        } else if (sky.Humidity != Humidity.High) {
            risk = 0.5;
        } else if (sky.Color == Color.Red) {
            risk = 0.8;
        }
    
        Log("value of risk:" + risk);
        return risk;
    

    }

  • If we're not talking about a method which returns something, but instead a if-else block which sets some variable or calls a method, then yes, I would include a final else clause.

Therefore, simplicity of the example is also a factor to consider.

4
  • I feel like you're making the same mistake others have made and morphing the example a little too much, thus examining a new issue, but I need to take a second and examine the process of getting to your example, so for now it looks valid to me. And a side note, this isn't related to the open/closed principle. The scope is much too narrow to apply this idiom. But it is interesting to note its application on a higher level would be removing this entire aspect of the problem (by removing the need for any modification of the function). – Selali Adobor Sep 6 '14 at 16:16
  • 1
    But if we cannot modify this over-simplified example adding clauses, nor modify how it's written, nor made any change to it, then you should consider that this question relates only to those precisely lines of code and it's not a general question anymore. In that case, you're completely right, it doesn't add any complexity (nor removes it) the else clause because there was no complexity to begin with. However, you're not being fair, because you're the one suggesting the idea that new clauses might be added. – FranMowinckel Sep 6 '14 at 16:43
  • And as I side note, I'm not stating this relates to the well known open/closed principle. I think that the idea of leading people to modify your code as you suggest is prone to errors. I'm just taking this idea from that principle. – FranMowinckel Sep 6 '14 at 16:52
  • Hold on on the edits, you're on to something good, I'm typing up a comment right now – Selali Adobor Sep 6 '14 at 17:02
1

Although the if-else case described has the greater complexity, in most (but not all) practical situations it does not matter much.

Years ago when I was working on software that was used for the aviation industry (it had to go through a certification process), yours was a question that came up. It turned out that there was a financial cost to that 'else' statement as it increased the code complexity.

Here's why.

The presence of the 'else' created an implicit 3rd case that had to be evaluated--that of neither the 'if' nor the 'else' being taken. You might be thinking (like I was at the time) WTF?

The explanation went like this ...

From a logical standpoint, there are only the two options--the 'if' and the 'else'. However, what we were certifying was the object file the compiler was generating. Therefore, when there was an 'else', an analysis had to be done to confirm that the generated branching code was correct and that a mysterious 3rd path did not appear.

Contrast this to the case where there is only an 'if' and no 'else'. In that case, there are only two options--the 'if' path and the 'non-if' path. Two cases to evaluate is less complex than three.

Hope this helps.

3
  • in an object file, wouldn't either case be rendered as identical jmps? – Winston Ewert Sep 6 '14 at 5:48
  • @WinstonEwert - One would expect them to be the same. However, what is rendered and what one expects to be rendered are two different things, regardless of how much they overlap. – Sparky Sep 6 '14 at 6:10
  • (I guess SE ate my comment...) This is a very interesting answer. While I'd say that the case it exposes is somewhat niche, it does show that some tools would find a difference between the two (Even the most common code metric based on flow I see,cyclomatic complexity, would measure no difference. – Selali Adobor Sep 6 '14 at 15:16
1

The most important goal of code is to be understandable as committed (as opposed to easily refactored, which is useful but less important). A slightly more complex Python example can be used to illustrate this:

def value(key):
    if key == FROB:
        return FOO
    elif key == NIK:
        return BAR
    [...]
    else:
        return BAZ

This is pretty clear - it's the equivalent of a case statement or dictionary lookup, the higher-level version of return foo == bar:

KEY_VALUES = {FROB: FOO, NIK: BAR, [...]}
DEFAULT_VALUE = BAZ

def value(key):
    return KEY_VALUES.get(key, default=DEFAULT_VALUE)

This is clearer, because even though the number of tokens is higher (32 vs 24 for the original code with two key/value pairs), the semantics of the function is now explicit: It is just a lookup.

(This has consequences for refactoring - if you wanted to have a side effect if key == NIK, you have three choices:

  1. Revert to the if/elif/else style and insert a single line in the key == NIK case.
  2. Save the lookup result to a value and add an if statement to value for the special case before returning.
  3. Put an if statement in the caller.

Now we see why the lookup function is a powerful simplification: It makes the third option obviously the simplest solution, since in addition to resulting in a smaller difference than the first option it's the only one which makes sure that value does only one thing.)

Going back to OP's code, I think this can serve as a guide for the complexity of else statements: Code with an explicit else statement is objectively more complex, but more important is whether it makes the semantics of the code clear and simple. And that has to be answered on a case by case basis, since every piece of code has a different meaning.

3
  • I like your lookup table rewrite of the simple switch case. And I know that's a favored Python idiom. In practice, however, I often find that multi-way conditionals (aka case or switch statements) are less homogenous. Several options are just straight value returns, but then one or two cases need extra processing. And when you discover the lookup strategy doesn't suit, you need to rewrite in an entirely different if elif ... else syntax. – Jonathan Eunice Sep 6 '14 at 12:54
  • That's why I think the lookup usually should be a one-liner or a function, so that the logic around the lookup result is separated from the logic of looking it up in the first place. – l0b0 Sep 6 '14 at 13:22
  • This question brings a higher level view to the question that definitely should be mentioned and kept in mind, but I feel that enough of a constraint has been placed on the case that you can feel free to expand on your own stance – Selali Adobor Sep 6 '14 at 15:12
1

I think it depends on what kind of if statement you're using. Some if statements can be looked at as expressions, and others can only be seen as control-flow statements.

Your example looks like an expression to me. In C-like languages, the ternary operator ?: is very much like an if statement, but it is an expression, and the else part can't be omitted. It makes sense that an else branch can't be omitted in an expression, because the expression must always have a value.

Using the ternary operator, your example would look like this:

bool CanLeaveWithoutUmbrella()
{
    return (sky.Color == Color.Blue)
               ? true
               : false;
}

Since it's a boolean expression, though, it really ought to be simplified all the way to

bool CanLeaveWithoutUmbrella()
{
    return (sky.Color == Color.Blue);
}

Including an explicit else clause makes your intent clearer. Guards can make sense in some situations, but in selecting between two possible values, neither of which is exceptional or unusual, they don't help.

7
  • +1, the OP is IMHO discussing a pathological case which should be better written the way you showed above. The much more frequent case for "if" with "return" is to my experience the "guard" case. – Doc Brown Sep 6 '14 at 12:19
  • I don't know why this keeps happening, but people keep ignoring the very first paragraph of the question. In fact, you're all using the exact line of code I explicitly say not to use. I ask that you ignore the many other ways the function can be written, and changes that can be made to it. (I know most people are itching to write return sky.Color == Color.Blue.) – Selali Adobor Sep 6 '14 at 15:00
  • And re-reading your last question you seem to misunderstand the intent of the question. – Selali Adobor Sep 6 '14 at 15:01
  • You used an example which was practically begging to be simplified away. I read and paid attention to your first paragraph, which is why my first code block is there at all, instead of skipping directly to the best way to do that particular example. If you think I (or anyone else) has misunderstood the intent of your question, maybe you should edit it to make your intent clear. – Michael Shaw Sep 6 '14 at 15:22
  • I would edit it, but I wouldn't know how to make it any more clear since I use the exact line of code you do and say, "don't do this". There are infinite ways you could rewrite the code and remove the question, I can't cover them all, yet so many people have understood my statement and respected it... – Selali Adobor Sep 6 '14 at 15:58
1

One other thing to think about in this argument is the habits each approach promotes.

  • if/else reframes a coding sample in the terms of branches. As you say, sometimes this explicitness is good. There really are two possible execution paths and we want to highlight that.

  • In other cases, there's only one execution path and then all those exceptional things that programmers must worry about. As you mentioned, guard clauses are great for this.

  • There is, of course, the 3 or more case (n), where we usually encourage abandoning the if/else chain and using a case/switch statement or polymorphism.

The guiding principle I keep in mind here is that a method or function should have only one purpose. Any code with an if/else or switch statement is for branching only. So it should be absurdly short (4 lines) and only delegate to the correct method or produce the obvious result (like your example). When your code is that short, it's hard to miss those multiple returns :) Much like "goto considered harmful", any branching statement can be abused, so putting some critical thought into else statements is usually worth it. As you mentioned, just having an else block provides a place for code to clump. You and your team will have to decide how you feel about that.

What the tool is really complaining about is the fact that you have a return/break/throw inside the if block. So as far as it's concerned, you wrote a guard clause but screwed it up. You can choose to make the tool happy or you can "entertain" its suggestion without accepting it.

2
  • I never thought about the fact the tool might be considering the if to be a guard clause as soon as it fits into the pattern for one, that's probably the case, and it explains the reasoning for one "group of proponents" for it's absence – Selali Adobor Sep 6 '14 at 15:37
  • @AssortedTrailmix Right, you're thinking about else semantically, code analysis tools usually look for extraneous instructions because they often highlight a misunderstanding of control flow. The else after an if that returns is wasted bits. if(1 == 1) often triggers the same sort of warning. – Steve Jackson Sep 6 '14 at 16:10
1

I think the answer is it depends. Your code sample (as you indirectly point out) is simply returning the evaluation of a condition, and is best represented as you obviously know, as a single expression.

In order to determine whether adding an else condition clarifies or obscures the code, you need to determine what the IF/ELSE represents. Is it a (as in your example) an expression? If so, that expression should be extracted and used. Is it a guard condition? If so, then the ELSE is unnecessary and misleading, as it makes the two branches appear equivalent. Is it an example of procedural polymorphism? Extract it. Is it setting preconditions? Then it can be essential.

Before you decide to include or elimitate the ELSE, decide what it represents, that will tell you whether it should be present or not.

0

The answer is a bit subjective. An else block can aid readability by establishing that one block of code executes exclusively to another block of code, without needing to read through both blocks. This can be helpful if, say, both the blocks are relatively long. There are cases, though where having ifs without elses can be more readable. Compare the following code with a number of if/else blocks:

if(a == b) {
    if(c <= d) {
        if(e != f) {
            a.doSomeStuff();
            c.makeSomeThingsWith(b);
            f.undo(d, e);
            return 9;
        } else {
            e++;
            return 3;
        }
    } else {
        return 1;
    }
} else {
    return 0;
}

with:

if(a != b) {
    return 0;
}

if(c > d) {
    return 1;
}

if(e != f) {
    e++;
    return 3;
}

a.doSomeStuff();
c.makeSomeThingsWith(b);
f.undo(d, e);
return 9;

The second block of code changes the nested if statements into guard clauses. This reduces indentation, and makes some of the return conditions clearer ("if a != b, then we return 0!")


It has been pointed out in the comments that there may be some holes in my example, so I will flesh it out a little more.

We could have written out the first block of code here like so to make it more readable:

if(a != b) {
    return 0;
} else if(c > d) {
    return 1;
} else if(e != f) {
    e++;
    return 3;
} else {
    a.doSomeStuff();
    c.makeSomeThingsWith(b);
    f.undo(d, e);
    return 9;
}

This code is equivalent to both the blocks above, but it presents some challenges. The else clause here connects these if statements together. In the guard clause version above, the guard clauses are independent of each other. Adding a new one means simply writing a new if between the last if and the rest of the logic. Here, that can also be done, with only a slight amount more cognitive load. The difference, though, is when some additional logic needs to occur before that new guard clause. When the statements were independent, we could do:

...
if(e != f) {
    e++;
    return 3;
}

g.doSomeLogicWith(a);

if(!g.isGood()) {
    return 4;
}

a.doSomeStuff();
...

With the connected if/else chain, this is not so easy to do. As a matter of fact, the easiest path to take would be to break up the chain to look more like the guard clause version.

But really, this makes sense. Using if/else weakly implies a relationship between parts. We could surmise that a != b is somehow related to c > d in the if/else chained version. That supposition would be misleading, as we can see from the guard clause version that they are, in fact, not related. That means that we can do more with independent clauses, like rearrange them (perhaps to optimize query performance or to improve logical flow). We can also cognitively ignore them once we are past them. In an if/else chain, I am less sure that the equivalence relationship between a and b won't pop back up later.

The relationship implied by else is also apparent in the deeply-nested example code, but in a different way. The else statements are separated from the conditional they are attached to, and they are related in reverse order to the conditionals (the first else is attached to the last if, the last else is attached to the first if). This creates a cognitive dissonance where we have to backtrack through the code, trying to line up indentation in our brains. It's a little like trying to match up a bunch of parenthesis. It gets even worse if the indentation is wrong. Optional braces don't help either.

I forgot to mention a case in which I agree with the omission of an else block, and is when using an if block to apply a constraint that must be logically satisfied for all following code, such as a null-check (or any other guards).

This is a nice concession, but it really doesn't invalidate any answer. I could say that in your code example:

bool CanLeaveWithoutUmbrella()
{
    if(sky.Color != Color.Blue)
    {
        return false;
    }
    return true;
}

a valid interpretation for writing code like this could be that "we only must leave with an umbrella if the sky is not blue." Here there is a constraint that the sky must be blue for the code following to be valid to run. I've met the definition of your concession.

What if I say that if the sky color is black, it's a clear night, so no umbrella necessary. (There are simpler ways to write this, but there are simpler ways to write the whole example.)

bool CanLeaveWithoutUmbrella()
{
    if(sky.Color == Color.Blue)
    {
        return true;
    }

    if(sky.Color == Color.Black)
    {
        return true;
    }

    return false;
}

As in the larger example above, I can simply add the new clause without much thought necessary. In an if/else, I can't be so sure I won't mess something up, especially if the logic is more complicated.

I will also point out that your simple example is not so simple, either. A test for a non-binary value is not the same as the inverse of that test with inverted results.

4
  • 1
    My edit to the question addresses this case. When a constraint that must be logically satisfied for all following code, such as a null-check (or any other guards) I agree the omission of an else block is essential for readability. – Selali Adobor Sep 6 '14 at 3:16
  • 1
    Your first version is difficult to grasp, but I don't think that's necessitated by using if/else. See how I'd write it: gist.github.com/winstonewert/c9e0955bc22ce44930fb. – Winston Ewert Sep 6 '14 at 4:04
  • @WinstonEwert See edits for response to your comments. – cbojar Sep 6 '14 at 5:50
  • Your edit makes some valid points. And I certainly do think that guard clauses have a place. But in the general case, I'm in favor of else clauses. – Winston Ewert Sep 6 '14 at 6:25
0

You have simplified your example too much. Either alternative may be more readable, depending on the larger situation: specifically, it depends on whether one of the two branches of the condition is abnormal. Let me show you: in a case where either branch is equally likely, ...

void DoOneOfTwoThings(bool which)
{
    if (which)
        DoThingOne();
    else
        DoThingTwo();
}

... is more readable than ...

void DoOneOfTwoThings(bool which)
{
    if (which) {
        DoThingOne();
        return;
    }
    DoThingTwo();
}

... because the first one exhibits parallel structure and the second one doesn't. But, when the bulk of the function is devoted to one task and you just need to check some preconditions first, ...

void ProcessInputOfTypeOne(String input)
{
    if (InputIsReallyTypeTwo(input)) {
        ProcessInputOfTypeTwo(input);
        return;
    }
    ProcessInputDefinitelyOfTypeOne(input);

}

... is more readable than ...

void ProcessInputOfTypeOne(String input)
{
    if (InputIsReallyTypeTwo(input))
        ProcessInputOfTypeTwo(input);
    else
        ProcessInputDefinitelyOfTypeOne(input);
}

... because deliberately not setting up parallel structure provides a clue to a future reader that getting input that's "really type two" here is abnormal. (In real life, ProcessInputDefinitelyOfTypeOne would not be a separate function, so the difference would be even more stark.)

8
  • It might be better to pick a more concrete example for the second case. My immediate reaction to it is, "it can't be that abnormal if you went ahead and processed it anyways." – Winston Ewert Sep 6 '14 at 16:59
  • @WinstonEwert abnormal is perhaps the wrong term. But I agree with Zack, that if you have a default (case1) and one exception, it should be written as »if->Do the exceptional and leave« as a return-early strategy. Think of code, in which the default includes more checks, it would be faster to handle the exceptional case first. – Thomas Junk Sep 6 '14 at 17:06
  • This seems to be falling into the case I was talking about with when using an if block to apply a constraint that must be logically satisfied for all following code, such as a null-check (or any other guards). Logically, any work ProcessInputOfTypeOne does depends on the fact that !InputIsReallyTypeTwo(input), so we need to catch that outside of our normal processing. Normally ProcessInputOfTypeTwo(input); would really be throw ICan'tProccessThatException which would make the similarity more obvious. – Selali Adobor Sep 6 '14 at 17:35
  • @WinstonEwert I could maybe have phrased that better, yes. I was thinking of a situation that comes up a lot when coding tokenizers by hand: in CSS, for instance, the character sequence "u+" usually introduces a "unicode-range" token, but if the next character isn't a hex digit, then one needs to back up and process the 'u' as an identifier instead. So the main scanner loop dispatches to "scan a unicode-range token" somewhat imprecisely, and it begins with "... if we are in this unusual special case, back up, then call the scan-an-identifier subroutine instead." – zwol Sep 6 '14 at 18:19
  • @AssortedTrailmix Even if the example I was thinking of hadn't been from a legacy codebase in which exceptions may not be used, this is not an error condition, it's just that the code that calls ProcessInputOfTypeOne uses an imprecise-but-fast check that sometimes catches type two instead. – zwol Sep 6 '14 at 18:22
0

Yes, there is an increase in complexity because the else clause is redundant. It's thus better to leave out to keep the code lean and mean. It's on the same chord as omitting redundant this qualifiers (Java, C++, C#) and omitting parenthesises in return statements, and so on.

A good programmer thinks "what can I add?" while a great programmer thinks "what can I remove?".

2
  • 1
    When I see the omission of the else block, if it's explained in a one-liner (which isn't often) it's often with this sort of reasoning, so +1 for presenting the "default" argument I see, but I don't find it to be a strong enough reasoning. A good programmer things "what can I add?" while a great programmer things "what can I remove?". seems to be a common adage that a lot of people overextend without careful consideration, and I personally find this to be one such case. – Selali Adobor Sep 6 '14 at 15:06
  • Yeah, reading your question I immediately felt you'd already made up your mind, but wanted confirmation. This answer is obviously not what you wanted, but sometimes its important to consider opinions that you don't agree with. Maybe there is something too it? – vidstige Sep 7 '14 at 0:53
-1

I noticed, changing my mind on this case.

When asked this question about a year ago, I would have answered something like:

»There should be only one entryand one exit to a method« And with this in mind, I would have suggested to refactor the code to:

bool CanLeaveWithoutUmbrella()
{
    bool result

    if(sky.Color == Color.Blue)
    {
        result = true;
    }
    else
    {
        result = false;
    }

    return result;
}

From a communication viewpoint it is clear, what should be done. If something is the case, manipulate the result in one way, else manipulate it in another way.

But this involves an unnecessary else. The else expresses the default-case. So in a second step, I could refactor it to

bool CanLeaveWithoutUmbrella()
{
    bool result=false

    if(sky.Color == Color.Blue)
    {
        result = true;
    }
    return result;
}

Then the question raises: Why using a temporary variable at all? The next step of refactorization leads to:

bool CanLeaveWithoutUmbrella()
{

    if(sky.Color == Color.Blue)
    {
        return true;
    }
    return false;
}

So this is clear in what it does. I would even go one step further:

bool CanLeaveWithoutUmbrella()
{

    if(sky.Color == Color.Blue) return true;
    return false;
}

Or for the faint-hearted:

bool CanLeaveWithoutUmbrella()
{

    if(sky.Color == Color.Blue) { return true; }
    return false;
}

You could read it like that: » CanLeaveWithoutUmbrella returns a bool. If nothing special happens, it returns false« This is the first read, from top to bottom.

And then you "parse" the condition »If the condition is met, it returns true« You could completely skip the conditional and have understood, what this function does in the default-case. Your eye and mind only have to skim through some letters on the left side, without reading the part on the right.

I'm under the impression the else more directly states intent, by stating the fact that the code in both blocks is directly related.

Yes, that is right. But that information is redundant. You have a default case and one "exception" which is covered by the if-statement.

When dealing with complexer structures, I would choose another strategy, omitting the if or it's ugly brother switch at all.

4
  • +1 redundancy definitely shows an increase in not just complexity, but confusion. I know I've always ended up having to double check code written like this exactly because of the redundancy. – dietbuddha Sep 6 '14 at 14:43
  • 1
    Could you remove the cases after the case starting with So this is clear in what it does... and expand on it? (The cases before are also of questionable relevance). I say this because it's what the question is asking we examine. You've stated your stance, omitting the else block, and it's actually the stance that needs more explanation (and the one that got me to ask this question). From the question: I ask that you ignore the many other ways the function can be written, and changes that can be made to it. (I know most people are itching to write return sky.Color == Color.Blue.) – Selali Adobor Sep 6 '14 at 15:22
  • @AssortedTrailmix Sure! On what exactly should I elaborate? Since your initial question was »Do else blocks increase code complexity?« and my answer is: yes. In this case it could be omitted. – Thomas Junk Sep 6 '14 at 16:54
  • And: No! I do not understand the downvote. – Thomas Junk Sep 6 '14 at 17:07
-1

To make a long story short, in this case, getting rid of the else keyword is a good thing. It's totally redundant here, and depending on how you format your code, it will add one to three more completely useless lines to your code. Consider what's in the if block:

return true;

Therefore if the if block were to be entered into, the whole rest of the function is, by definition, not executed. But if it is not entered into, since there are no further if statements, the whole rest of the function is executed. It would be totally different if a return statement were not in the if block, in which case the presence or absence of an else block would have an impact, but here, it would not have an impact.

In your question, after inverting your if condition and omitting else, you stated the following:

Someone can now add a new if block based on a condition after the first example without immediately correctly recognizing the first condition is placing a constraint on their own condition.

They need to read code a little bit more closely then. We use high-level languages and clear organization of code to mitigate these issues, but adding a completely redundant else block adds "noise" and "clutter" to the code, and we shouldn't have to invert or re-invert our if conditions either. If they can't tell the difference, then they don't know what they're doing. If they can tell the difference, then they can always invert the original if conditions themselves.

I forgot to mention a case in which I agree with the omission of an else block, and is when using an if block to apply a constraint that must be logically satisfied for all following code, such as a null-check (or any other guards).

That's essentially what's happening here though. You're returning from the if block, so that if condition evaluating to false is a constraint that must be logically satisfied for all following code.

Is there an increase in complexity introduced by not including the else block?

No, it will constitute a decrease in complexity in your code, and it may even constitute a decrease in the compiled code, depending on whether certain super trivial optimizations will or will not take place.

2
  • 2
    "They need to read code a little bit more closely then." Coding style exists so that programmers can pay less attention to nitpicky details and more attention to what they're actually doing. If your style makes you pay attention to those details unnecessarily, it's a bad style. "...completely useless lines..." They aren't useless -- they let you see at a glance what the structure is, without having to waste time reasoning about what would happen if this part or that part were executed. – Michael Shaw Sep 6 '14 at 19:07
  • @MichaelShaw I think Aviv Cohn's answer goes into what I'm talking about. – Panzercrisis Jun 5 '15 at 16:00
-2

In the specific example you gave, it does.

  • It forces a reviewer to read the code again to make sure it's not an error.
  • It adds (unnecessary) cyclomatic complexity because it adds one node to the flow graph.

I think it couldn't get any simpler that this:

bool CanLeaveWithoutUmbrella()
{
    return (sky.Color == Color.Blue);
}
1
  • 6
    I specifically address the fact this very simplified example can be rewritten to remove the if statement. In fact I explicitly discourage further simplification using the exact code you used. Additionally the cyclomatic complexity is not increased by the else block. – Selali Adobor Sep 6 '14 at 2:51
-4

I always try to write my code such that the intention is as clear as possible. Your example is lacking some context, so I'll modify it a little:

class Sky {
    Sky(Colour c)
    {
        this.color = c;
    }
    bool CanLeaveWithoutUmbrella()
    {
        if(this.color == Color.Blue)
        {
            return true;
        }
        else
        {
            return false;
        }
    }
}

Our intention seems to be that we can leave without an umbrella when the Sky is Blue. However, that simple intention is lost in a sea of code. There are several ways we can make it easier to understand.

Firstly, there's your question about the else branch. I don't mind either way, but tend to leave out the else. I understand the argument about being explicit, but my opinion is that if statements should wrap 'optional' branches, like the return true one. I wouldn't call the return false branch 'optional', since it's acting as our default. Either way, I see this as a very minor issue, and far less important than the following ones:

class Sky {
    Sky(Colour c)
    {
        this.color = c;
    }
    bool CanLeaveWithoutUmbrella()
    {
        if(this.color == Color.Blue)
        {
            return true;
        }
        return false;
    }
}

A much more important issue is that your branches are doing too much! Branches, like everything, should be 'as simple as possible, but no more'. In this case you've used an if statement, which branches between code blocks (collections of statements) when all you really need to branch between are expressions (values). This is clear since a) your code blocks only contain single statements and b) they are the same statement (return). We can use the ternary operator ?: to narrow down the branch to only the part that's different:

class Sky {
    Sky(Colour c)
    {
        this.color = c;
    }
    bool CanLeaveWithoutUmbrella()
    {
        return (this.color == Color.Blue)
            ? true
            : false;
    }
}

Now we reach the obvious redundancy that our result is identical to this.color == Color.Blue. I know your question asks us to ignore this, but I think it's really important to point out that this is a very first-order, procedural way of thinking: you're breaking down the input values and constructing some output values. That's fine, but if possible it's much more powerful to think in terms of relationships or transformations between the input and the output. In this case the relationship is obviously that they're the same, but I often see convoluted procedural code like this, which obscures some simple relationship. Let's go ahead and make this simplification:

class Sky {
    Sky(Colour c)
    {
        this.color = c;
    }
    bool CanLeaveWithoutUmbrella()
    {
        return (this.color == Color.Blue);
    }
}

The next thing I'd point out is that your API contains a double-negative: it's possible for CanLeaveWithoutUmbrella to be false. This, of course, simplifies to NeedUmbrella being true, so let's do that:

class Sky {
    Sky(Colour c)
    {
        this.color = c;
    }
    bool NeedUmbrella()
    {
        return (this.color != Color.Blue);
    }
}

Now we can start to think about your coding style. It looks like you're using an Object Oriented language, but by using if statements to branch between code blocks, you've ended up writing procedural code.

In Object Oriented code, all code blocks are methods and all branching is done via dynamic dispatch, eg. using classes or prototypes. It's arguable whether that makes things simpler, but it should make your code more consistent with the rest of the language:

class Sky {
    bool NeedUmbrella()
    {
        return true;
    }
}

class BlueSky extends Sky {
    bool NeedUmbrella()
    {
        return false;
    }
}

Note that using ternary operators to branch between expressions is common in functional programming.

7
  • The first paragraph of the answer seems to be on track to answering the question, but it immediately diverges into the exact topic I explicitly ask one ignores for this very simple example . From the question: I ask that you ignore the many other ways the function can be written, and changes that can be made to it. (I know most people are itching to write return sky.Color == Color.Blue.). In fact you use the exact line of code I mention. Additionally, while this part of the question is off-topic, I'd say you're going too far in your inference. You've radically changed what the code is. – Selali Adobor Sep 6 '14 at 14:57
  • @AssortedTrailmix This is purely a question of style. It makes sense to care about style and it makes sense to ignore it (focusing only on results). I don't think it makes sense to care about return false; vs else { return false; } whilst not caring about branch polarity, dynamic dispatch, ternaries, etc. If you're writing procedural code in an OO language, radical change is necessary ;) – Warbo Sep 6 '14 at 15:27
  • You can say something like that in general, but it's not applicable when you're answering a question with well-defined parameters (especially when without respecting those parameters you can "abstract away" the entire question). I'd also say that last sentence is just plain wrong. OOP is about "procedural abstraction", not "procedural obliteration". I'd say the fact you went from a one liner to an infinite number of classes (One for each color) shows why. Additionally I'm still wondering what dynamic dispatch should have to do with an if/else statement, and what branch polarity is. – Selali Adobor Sep 6 '14 at 15:50
  • I never said the OO solution is better (in fact, I prefer functional programming), I merely said it's more consistent with the language. By "polarity" I meant which thing is "true" and which is "false" (I switched it with "NeedUmbrella"). In OO design, all control flow is determined by dynamic dispatch. For example, Smalltalk doesn't allow anything else en.wikipedia.org/wiki/Smalltalk#Control_structures (also see c2.com/cgi/wiki?HeInventedTheTerm ) – Warbo Sep 6 '14 at 16:14
  • 2
    I would have upvoted this till I got to the very last paragraph (about OO), which earned it a downvote instead! That's exactly the sort of unthinking overuse of OO constructs that produces ravioli code, which is just as unreadable as spaghetti. – zwol Sep 6 '14 at 16:29

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