8

1) I know how if…else if statements work, but in the next example both methods are identical as far as the resulting value is concerned. So does it matter which of the two methods I use or should I always go for one that is semantically closest to what the code is trying to do ( here I’m guessing that semantically the two methods are quite different )? So which method would you use and why?

protected string GetNumberDescription(int value)
{
    if (value >= veryBigNumber)
        return veryBigNumberDescription;
    else if (value >= bigNumber)
        return bigNumberDescription;
    else if (value >= smallNumber)
        return smallNumberDescription;
    else
        return "";
}

protected string GetNumberDescription(int value)
{
    if (value >= veryBigNumber)
        return veryBigNumberDescription;
    if (value >= bigNumber)
        return bigNumberDescription;
    if (value >= smallNumber)
        return smallNumberDescription;
    else
        return "";
}

2) I noticed losts of code uses the following format when writting if ... else if statements:

if ...
else if ...
else ...

But isn't ( at least conceptually ) more correct way:

if ...
else
  if ...
  else ...
GSto
  • 41,512
  • 37
  • 133
  • 184
user437291
  • 4,561
  • 7
  • 37
  • 53
  • 1
    Similar question in Java context: http://stackoverflow.com/questions/3579918/programming-preference-use-else-ifs-with-multiple-return-statements/ – BalusC Sep 02 '10 at 19:31
  • 1
    Check out http://www.refactoring.com/catalog/replaceNestedConditionalWithGuardClauses.html... – Per Noalt Sep 02 '10 at 19:32
  • Doing the first one can lead to issues when someone inserts an else if statement somewhere inside that is being handled by the block of ifs. The issue being code redundancy and performance hits – Woot4Moo Sep 02 '10 at 19:49
  • 2
    Just remember that if/else, like switch() often indicate bad OO. In your case you are forcing an edit of your if block every single time someone adds a new level of number--put them into a collection and iterate over it. – Bill K Sep 02 '10 at 21:07

18 Answers18

16
  1. You should probably use the first. It's more accepted, and more logical. (You don't care about anything afterwards if this condition is true... indicate that in your code.)
  2. It's generally more accepted, and more compact and readable, to use the first way (else if). Python even has a specific keyword for it (elif).
froadie
  • 79,995
  • 75
  • 166
  • 235
  • 1
    The first doesn't save unnecessary evaluations as each true condition ends with a return statement. – Michael La Voie Sep 02 '10 at 19:28
  • @Michael La Voie - yup, sorry, I had just realized my mistake and corrected it... I didn't immediately notice the returns – froadie Sep 02 '10 at 19:29
  • agreed, the second style is just bad coding. – Gabriel Magana Sep 02 '10 at 19:31
  • @froadie: Why is it "more compact" to use the else if? It's less compact because you have to type more. Readability is debatable, too. I do agree that the first style is probably more common, though. – Mark Simpson Sep 02 '10 at 19:34
  • @gmagana: Why do you consider it "bad coding"? They're functionally equivalent and equally readable in my eyes. Seems like a very subjective assessment to say one is "bad" without saying why. – Mark Simpson Sep 02 '10 at 19:36
  • @Mark Simpson - why do you have to type more? It's the same amount of words, just "else if" on one line instead of "else" on one line and "if" on the next. It's more compact because it's less lines – froadie Sep 02 '10 at 19:36
  • @Mark Simpson: I think froadie's comment about it being more compact was in response to the OP's second question. – Dan Tao Sep 02 '10 at 19:54
13

I personally would use

protected string GetNumberDescription(int value)
   {
       if (value >= veryBigNumber)
           return veryBigNumberDescription;
       if (value >= bigNumber)
           return bigNumberDescription;
       if (value >= smallNumber)
           return smallNumberDescription;
       return string.Empty;
   }

It really all depends on what your function is doing. If its simple like this then keep it simple. If you might need to do something with the result before you return it then I'd use Egrunin's solution

Gage
  • 7,365
  • 9
  • 47
  • 77
  • for(Number number : numberCollection) if(value >= number.value) return number.description; throw new IllegalArgumentException();----which then obviously should be a method that exists inside your NumberCollection class and this becomes numberCollection.getDescription(value); Don't be afraid to code well. (I'm not implying that the answer wasn't clearer/better for instructional purposes, just taking the next step) Also number is one of those words that if you type it too many times in a row it doesn't look right. – Bill K Sep 02 '10 at 21:00
12
protected string GetNumberDescription(int value) 
{ 
    string ret = "";

    if (value >= veryBigNumber) 
        ret = veryBigNumberDescription; 
    else if (value >= bigNumber) 
        ret = bigNumberDescription; 
    else if (value >= smallNumber) 
        ret =  smallNumberDescription; 

    return ret; 
} 

The question is already answered, but I added this because:

  1. There are advantages to having a single exit from a function.
  2. The same variable is being tested repeatedly, so the if/elseif is most appropriate.

Edited to add:

I'm usually a big fan of exiting a function early in cases like this:

if (conditionOne)
{
    if (conditionTwo)
    {
        if (conditionThree)
        {
            interesting_stuff();
        }
    }
    else
        boring();
}
return;

I'm much happier seeing this:

if (!conditionOne)
    return;

if (!conditionTwo)
{
    boring();
    return;
}

if (!conditionThree)
    return;

interesting_stuff();
return;

But when there are exits at many different nesting levels, it's too easy to miss one as you're reading over the code.

Also, as mentioned in the comments: a single exit means only one place to put a breakpoint.

egrunin
  • 24,650
  • 8
  • 50
  • 93
  • Could you expand on your first point? I would normally prefer otherwise, to make my code smaller. – Karmastan Sep 02 '10 at 21:10
  • 2
    Because a return acts as a control flow statement, making reading the code more complex. If you have return statements sprinkled all throughout a function body then you have more use scenarios to think about when trying to understand how the function works. If you try to modify the function and add more code to it, you are limited to placing the statements at the beginning of the method if you always want them to execute, because if you place them at the end then in some cases the statement won't get that far. But maybe you need to analyze the result, so you can't place it at the beginning. – AaronLS Sep 02 '10 at 21:36
  • 1
    +1 We can put ONE break point on the single return to check result. – garik Sep 03 '10 at 15:39
11

If the conditions are mutually exclusive then I think an else if pattern is best. It will be harder to accidentally break as the code evolves and becomes more complex, because your control flow more accurately represents the mutual exclusiveness of the logic.

Consider if someone re-factored your first example to be similar to egrunin's example such that there is only one return, but consider they forgot to add the "else" to each if:

protected string GetNumberDescription(int value) 
{ 
    string ret = "";

    if (value >= veryBigNumber) 
        ret = veryBigNumberDescription; 
    if (value >= bigNumber) 
        ret = bigNumberDescription; 
    if (value >= smallNumber) 
        ret =  smallNumberDescription; 
    // BUG: very big numbers still fall through to this last condition, 
    //         because they meet all conditions

    return ret; 
} 

They have just introduced a bug. They may not have realized that the returns were part of the logic being expressed. While the error is obvious here, in real life this code a couple years down the road could potentially become more complex, lengthy, and more difficult to read such that it's easier to make such a mistake.

Detecting Unexpected Bug

Additionally, if at least one condition must be met(i.e. should never return an empty string), then I'd throw an exception in the final else if you are assuming all cases should be handled. This would detect a bug in the code where the code fell through all the elses and did not find a matching condition. If you do not throw an exception, then the bug might cause strange behavior. How will the rest of the code handle the uninitialized empty string? Depends largely on the code at hand. The idea of throwing an exception is to cause a hard failure exactly where the bug is so that it can be easily identified and corrected, instead of allowing the application to continue running with an invalid return.

if (value >= smallNumber) 
    ret =  smallNumberDescription; 
else if(...)
    ret =  otherNumberDescription; 
else
    throw new InvalidOperationException($"No condition matched for value={value}");

return ret; 

If paired with appropriate exception logging/notifications, then this would allow you to become aware of this bug and add a condition to handle unanticipated values. Not really necessary for very simple conditions, but very helpful in proactively finding and correcting bugs in this kind of code.

AaronLS
  • 37,329
  • 20
  • 143
  • 202
7

They are semantically identical. I doubt there is any difference in performance, so the choice is about style and readability. If you're curious, compile the above code into an assembly and use Reflector to see if there is any difference in the IL. My guess is there would be little difference if any.

Dave Swersky
  • 34,502
  • 9
  • 78
  • 118
  • I think the poster realizes that the generated code will be equivalent. He's asking if there is a reason to prefer one *coding style* above the other. – Karmastan Sep 02 '10 at 21:11
  • @Karmastan: The OP states "semantically they are quite different." They are not semantically different. I don't hold a strong opinion on the stylistic differences, just pointing out that they mean the same thing. – Dave Swersky Sep 03 '10 at 13:41
6

It's mostly a matter of preference. when using return statements, you don't need the elses in my opinion, since it's clear that the function ends right there. It's all very situation and style dependent, so there is no clear 'best' way to do it.

GSto
  • 41,512
  • 37
  • 133
  • 184
  • +1 for saying that's situation/style dependent -- this is the sort of thing folk argue over when either is acceptable. – Mark Simpson Sep 02 '10 at 19:31
  • I agree to some extent. In this case clearly the conditions are interrelated and the else-if style is more defensive to bugs, so I am inclined to say this is less of a style issue. – AaronLS Sep 02 '10 at 19:48
5

I would suggest the former, as you are making a logical choice between four possibilities. You are grouping the decision in one if/else if block.

If you separate out the decisions like in your second function, they appear to not be connected at all. Clearly they are, as the value variable is repeated, but it might not necessarily be obvious.

Additionally, when you've got nothing to return, please return null, and not a blank string. In an application I maintain, it is still necessary to use if (value == null || value != "").

Steve Rukuts
  • 9,167
  • 3
  • 50
  • 72
5

You want to use "else if" because that only will fire if the first "if" is not accepted. In your case the 3 if statements are okay, simply because if a latter "if" is correct, then the previous "if"s cannot be.

In a case where all of your if statements could be correct, you'd want to use else if.

For Example:

if (8<10) {
return 10;
}
if (8<9) {
return 9;
}

That's a very simplistic example, but you get the idea. You use "else if" when you want only one "if" statement to fire.

Daniel G. Wilson
  • 14,955
  • 2
  • 32
  • 39
4

I would use the first option, as it shows in code the comparisons are "grouped" some. For 2 I think they end up in exactly the same IL code.

I always make sure the most common senario is first in the if..else statement, as that is best for performance.

Peter Kiers
  • 602
  • 4
  • 16
4

1) The first one is more prevalent and in common practice. So it would be wise to stick to the standard practice.

2) Generally when we have more than one conditional statement, else if is used in the intermediate conditional outputs and else is used in the last conditional output. Moreover it might be more efficient as I believe it does not have to check the same condition everytime unlike the if-else-if-else... type.

Its more like a choice provided to you where you find your own out of many paths going to the same destination. But the choice that you hear or see often is the one that is popular because of experience of many users and experts.

4

The alternatives are pretty close semantically, so it doesn't matter that much. As suggested in some answers, putting the result in a string variable and return at the end might better represent what you are doing in the method.

Another alternative is to use the conditional operand, which gives you a less verbose code:

protected string GetNumberDescription(int value) {
  return
    value >= veryBigNumber ? veryBigNumberDescription :
    value >= bigNumber ? bigNumberDescription :
    value >= smallNumber ? smallNumberDescription :
    String.Empty;
}
Guffa
  • 687,336
  • 108
  • 737
  • 1,005
  • I think this is harder to understand. – Daniel Moura Sep 06 '10 at 19:17
  • I have to think to understand this code. Maybe I'm not used to seeing this style. Using if...else if...else I understand immediately, I don't have to think about. I use and see code like this a>b ? c : d and I think is ok. When nesting the conditional operand I think is harder to figure out what is happening. – Daniel Moura Sep 08 '10 at 13:01
4

With the return statements, it really doesn't matter. I totally disagree with those who say the second is in any way worse, though. What makes it worse?

In response to your second question: you are quite right. In fact, not only are the two ways of formatting if/else if effectively the same; they actually are literally the same. C# does not enforce a particular formatting style, so these are all identical:

// This...
if (A())
    return "Congrats!";
else if (B())
    return "Not bad!";
else if (C())
    return "Next time...";
else
    return "Uh-oh.";

// ...is the same as this...
if (A())
    return "Congrats!";
else
    if (B())
        return "Not bad!";
    else
        if (C())
            return "Next time...";
        else
            return "Uh-oh.";

// ...which is the same as this...
if (A()) return "Congrats!"; else
                       if               (B())
             return "Not bad!";


else
{ if
(C())


     return


"Next time...";else{
return "Oh-oh.";
}}
Dan Tao
  • 125,917
  • 54
  • 300
  • 447
4
protected string GetNumberDescription(int value) 
{ 
    return new NumberDescriptor(value).toString();
} 

All you need is to hide your logic. Don't take this too serious, but...i would do it that way. I know, your question is not anserwed here, but there are already enough if/then/else examples in this topic.

InsertNickHere
  • 3,616
  • 3
  • 26
  • 23
3

I would say to use the first if the function is thought of as returning one of four possibilities, all of which are considered "successful". I would use the latter style if some returns denoted "failures". For example (toy example--throwing might be better for the error cases):

string get_suit_name(int suit)
{
  string suit_names[4] = {"clubs", "diamonds", "hearts", "spades"};

  if (0 > suit) /* Avoid LT sign */
    return "Suit is improperly negative!";
  if (suit >= 4)
    return "Suit is too big!";
  return suit_names[suit];
}
supercat
  • 77,689
  • 9
  • 166
  • 211
3

Well from my point of view it would be these priorities from highest to lowest.

Functionality / achieving the end goal Reduction of cost in any scope/level Readability

^^^^ note that many will dispute this on my list and call readability above reduction of cost, it really depends on your work envi. and the end goal but I find this the most common. But guess coming from a rookie that doesn't mean much huh :D

After that you it's pretty much peanuts. Same goes for ? : operator. If you are convinced it's more readable, go for it. Function wise it doesn't really matter. But if(){}else{} is significantly different from if(){}else if(){} So pick what's the optimum code (Highest wanted results minus unexpected results - 0.5* readability score ) pretty much what I'd go on.

Now seeing as the code does the same for you, a new attribute comes into play, will this code be edited at some later stage? And if so what would be best to go for then? Don't forget the future of the code, nor the comments, that a free bonus to pick along with it. You can even do both if you can't figure it out for the time and just comment the other but that does leave sloppy walls of text to be removed later on cleanup/release.

Proclyon
  • 526
  • 5
  • 12
3

In terms of performance, they may be the same depending on the compiler/interpreter. In my opinion the second one is better....much easier to debug.

Rabi
  • 2,593
  • 1
  • 23
  • 26
2

For the example provided, you don't need to worry about "if" statements if you're willing to sacrifice a ton of readability and possibly a little speed:

string [] descriptions = {"", smallNumberDescription, bigNumberDescription, veryBigNumberDescription};

protected string GetNumberDescription(int value)
{
    int index = Convert.ToInt32(value >= smallNumber) + Convert.ToInt32(value >= bigNumber)
                + Convert.ToInt32(value >= veryBigNumber);
    return descriptions[index];
}
TwoLineGuy
  • 29
  • 1
1

Well, in your particular case, I would suggest this:

    protected string GetNumberDescription(int value) {
        if (value >= veryBigNumber)
            return veryBigNumberDescription;
        if (value >= bigNumber)
            return bigNumberDescription;
        if (value >= smallNumber)
            return smallNumberDescription;

        return "";
    }

Since, you are returning from the function on any of the true condition, there is no need to use 'else if'. In your case, if any of the conditions become true, the code won't execute further. Also, the last 'else' is not required, for the control will reach that point only when all the above conditions have been false.

It's all a matter of where you're using it, and how. The main motive is to maximize the readability of your code and make it as elegant and easy-to-understand as possible.

Many would prefer the first way since it indicates in your code that you don't care what happens as soon as a condition is satisfied.