views:

397

answers:

15

The code:

public String getTemperatureMessage(double temp)
{
    if(temp < 32)
        return "Freezing";
    else if(temp < 60)
        return "Brr";
    else if(temp < 80)
        return "Comfortable";
    else
        return "Too hot";
}

Regarding the code snippet above, the else ifs are technically superfluous and don't change the behavior at all. However, I tend to like to put them in there to emphasize that the conditions are exclusive. What are your thoughts? Unnecessary or more clear?

+4  A: 

Some would say that multiples return would be the problem here. But it's not really my point.

For my point of view, the if/else if is really important, because even if in your case you return some value, removing the elses would mean that you wouldn't put them anyway, and that would mean a totally different thing if the returns were not here.

Plus, imagine someday someone want to edit your code, and clean it up for a single return, this person could misunderstand your code and do a grave mistake like this :

public String getTemperatureMessage(double temp){
    String message;
    if(temp < 32)
        message = "Freezing";
    if(temp < 60)
        message = "Brr";
    if(temp < 80)
        message = "Comfortable";
    else 
        message = "Too hot";
    return message;
}

To clarify my point of view, keep the elses, it keep your code clear.

Colin Hebert
He probably means something like public int foo { if () return 1; if () return 2; if () return 3; return 42;
Helper Method
-1: Multiple returns are not the problem and removing `else` s would not change the code as the method is exited when each condition is met.
Callum Rogers
I don't like the multiple returns. I like One Entry point, One Exit for clarity.
MikeAinOz
@Callum, I didn't said that returns were the problem. Read my post.
Colin Hebert
@Colin: Even so, this is a situation where having only one exit point leads to extra code clutter and complexity. You could make a simple mistake, like _forgetting to return the variable at the end of the method_. (Lol, you fixed it while I was typing :) )
Callum Rogers
@Callum, still not what I'm saying. (And IDEs are really better than textareas to write code).
Colin Hebert
OMG Ponies
OK, I'm I stupid and I didn't understand something or I'm getting downvotes because nobody read the OP nor my comment entirely ?
Colin Hebert
@OMG Ponies: I emphatically disagree (for a simple method like this) and @Colin: I see your point now, as in relation to the question it is a good answer (I was put off by what looked like SEPSEP dogma) so have your 2 rep back.
Callum Rogers
You're getting down votes because your decision to both remove the else's and remove the returns introduced a bug.Now everything is either "Comfortable" or "Hot".This is why you are better of either going for the pattern matching approach, ie. @StephenDenne's first option; or for the explicit conditional approach given in the question.
Recurse
@Recurse, read my post and the OP post! He asked if elses were useful, and I said yes with an example why! (You've got to be kidding me, has anyone really read the question ??)
Colin Hebert
+1: Because everyone else seems to be downvoting you when they really just didn't read at all. :D
David Liu
I think that the worse part is that some people vote even if they don't know what the question is.
Colin Hebert
A: 

I agree that the elses makes it more clear. The final else especially helps to make a visual distinction between the case where every branch has a return, and the cases where only some branches have a return (which could be a smell).

ILMTitan
+1  A: 
public String getTemperatureMessage(double temp)
{
    String retval = null;
    if(temp < 32)
        retval = "Freezing";
    else if(temp < 60)
        retval = "Brr";
    else if(temp < 80)
        retval = "Comfortable";
    else
        retval = "Too hot";
    return retval;
}
Paul Tomblin
Why are you assigning retval to null?
ILMTitan
Just out of habit to make sure it's always initialized to something.
Paul Tomblin
@Paul - in Java, its a bad idea to do this "automatically" (i.e. without thinking). It stops the java compiler pointing out bugs where you forgot to assign a (non-null) value in one of the paths through the code.
Stephen C
A: 

There's no point. You're adding needless semantic and other overheads for absolutely zero benefit. When you return, you return and control is over. Pretending anything else is superfluous and just makes you look like you don't know what the return statement does.

DeadMG
+9  A: 

The only feasible alternative in this particular case is to grab the conditional operator ?:.

public String getTemperatureMessage(double temp) {
    return temp < 32 ? "Freezing"
         : temp < 60 ? "Brr"
         : temp < 80 ? "Comfortable"
         : "Too hot";
}

Left behind the question how that's readable to starters.

References

Related questions

BalusC
Ha cool! I don't like it, but it's clever!
Shakedown
That is the ugliest ternary sentence I've ever seen
TheLQ
Ew, it could be worse, here there are only 4 choices
Colin Hebert
Btw, it's daily 85~90F [here](http://en.wikipedia.org/wiki/Curacao#Climate). *I* consider it comfortable :)
BalusC
@BalusC I use Celcius. @Quackstar I disagree, uglier would be all that on a single line. I think the way it is structured here adds clarity.
Stephen Denne
@Stephen: we also use Celsius here. The logic of the particular method however suggests that it's based on Fahrenheit.
BalusC
A: 

Who uses IF/ELSE IF... when you can use a SWITCH statement?

My preference is for the single RETURN statement - multiple return statements can make debugging a pain...

OMG Ponies
Switch cannot be used on a `double`. Even then, if it was an `int`, you would need to put **all** possible case statements...
BalusC
@BalusC: Crappy Java data type support aside, considering language agnosticism...
OMG Ponies
+9  A: 

It depends on a lot of things like how complex your code is. With a simple example like this, I'd put the returns on the same line as the ifs, and not use elses. The structure and behaviour is clear:

public String getTemperatureMessage(double temp)
{
    if(temp < 32) return "Freezing";
    if(temp < 60) return "Brr";
    if(temp < 80) return "Comfortable";
    return "Too hot";
}

When I have more complex code I find it useful to not break out from the nesting with returns or continue/break, but to assign to state or result variables. I will then also include {} even if the block is a single statement, mostly to keep the consistency in how the structure is represented in code, but also to slightly reduce the risk that later edits will forget to change a statement to a block.

If this example was more complex, I'd probably code it like this:

public String getTemperatureMessage(double temp) {
    String result;
    if(temp < 32) {
        result = "Freezing";
    } else {
        if(temp < 60) {
            result = "Brr";
        } else {
            if(temp < 80) {
                result = "Comfortable";
            } else {
                result = "Too hot";
            }
        }
    }
    return result;
}
Stephen Denne
A: 

I don't think I'd write in this way in the first place, but going along with the premise, I'm guessing that any compiler would crank out the same code whichever method you chose to write, so there is no technical reason that I can think would favour one over the other.

The argument is therefore Single or Compound statement.

I think the rule of "least surprise" should be used, which in this case would call FOR the superfluous else statements to be included.

PS. I would always send a temperature in degrees centigrade, oops, just broke your function!

blissapp
+2  A: 

For simple 1-liners I tend to leave out the else but if there's more complex if blocks I tend to prefer the else to make it clear that the conditions are mutually exclusive.

Davy8
+7  A: 

If a function has multiple "successful" return values, I'll use if/else to select among them. If a function has a normal return value, but one or more ways that may abnormally exit early, I generally won't use an "else" for the normal path. For example, I think it's far more "natural" to say:

int do_something(int arg1)
{
  if (arg1 > MAX_ARG1_VALUE)
    return ARG1_ERROR;
  ... main guts of code here
  return 0;
}

than to say:

int do_something(int arg1)
{
  if (arg1 > MAX_ARG1_VALUE)
    return ARG1_ERROR;
  else
  {
    ... main guts of code here
    return 0;
  }
}

or

int do_something(int arg1)
{
  if (arg1 <= MAX_ARG1_VALUE)
  {
    ... main guts of code here
    return 0;
  }
  else
    return ARG1_ERROR;

This distinction becomes especially significant if there are multiple things that can "go wrong", e.g.

int do_something(int arg1)
{
  if (arg1 > MAX_ARG1_VALUE)
    return ARG1_ERROR;
  ... some code goes here
  if (something_went_wrong1)
    return SOMETHING1_ERROR;
  ... more code goes here
  if (something_went_wrong2)
    return SOMETHING2_ERROR;
  ... more code goes here
  if (something_went_wrong3)
    return SOMETHING3_ERROR;
  return 0;
}

Nested 'if/else' statements in such cases can get ugly. The most important caveat with this approach is that any cleanup code for early exits must be given explicitly, or else a wrapper function must be used to ensure cleanup.

supercat
Great answer. The first paragraph sums it up for me.
John Kugelman
@John Kugelman: Thanks. I was just thinking of another point: if the last two lines of a function are a conditional return and an unconditional return, that tends to suggest that the function may have additional exits. If I have a function end with two conditional returns (both of which are "successful") but there are other unsuccessful returns as well, I'll add a comment to the effect of "If we get here..." as a hint to look for other exits.
supercat
A: 

it's good. without "else", this line

if(temp < 80)
    return "Comfortable";

would be unconvincing. with "else", it's clear that there are other preconditions.

irreputable
A: 

I prefer case statements myself but that would make this a language specific topic and not language-agnostic topic.

    Dim Temp As Integer
    Dim Message As String

    Select Case Temp
        Case Is < 32
            Message = "Freezing"
        Case Is < 60
            Message = "Just Right"
        Case Is < 80
            Message = "Too Hot"
        Case Else
            Message = "What was I doing?"
    End Select

I find these alot easier to read then if..else statements.

Varuuknahl
+1  A: 

For a simple if statement without too many lines of code having multiple returns is no problem. However, nothing infuriates me quite so much as:

function doTemperatureCalculations(double temperature) {
  if (temperature < 20) {
    /* 
      Gazillion lines of code here .....
     */
    return "Very cold!";
  } else if (temperature < 40) {
    /*
      Another gazillion loc .....
     */
    return "Summer in the North Pole.";
  } else {
    /*
      Multiple returns embedded throughout ....
     */
  }
}
pygorex1
I agree, if you are going to have multiple returns, make them easy to find. However the question is whether to bother to use `else` when the if block always ends with `return`
Stephen Denne
+1  A: 

In this case it is more clear. In the general case you may want to leave the elses off as they can contribute to more nesting and code complexity. For example:


if (error condition) {  
  do some stuff;
  return;
} else {
  do stuff;
  if (other error condition) {
     do some stuff1;
     return;
  } else {
     do some other stuff;
     return

  }
}

The code below keeps the level of nesting down which reduces code complexity:


if (error condition) {
  do some stuff;
  return;
}
do stuff;
if (other error condition) {
  do some stuff1;
  return;
}
do some other stuff;
return;

In your example it is easy either way. But in many cases you would be better off using a lookup table for this sort of thing and reading the values from a file/database. For efficiency in C often this would be coded as an array of structs.

The else does add some clarity in that it makes it clear that the cases are mutually exclusive. However the idiom of returning like you do is obvious to many programmers, so either way the majority will know what you meant.

I can think of an advantage of the elses. If you want to add a new last case, without the elses you might forget to add an if to the currently "Too Hot Condition" if say you wanted to add "Dying" at 120 or something. while with the elses you know that you need the final else to be in front of "Dying" so you will be more likely to think about putting the else if in front of "Too Hot". Also if you just put an else on "Dying" you will get a compile error which forces you to think.

Cervo
+1  A: 

Personally, I think the elses are unnecessary. Since this question is tagged as [language-agnostic], I'm going to provide a couple of examples of how I would write it:

def temperature_message(temp)
  return 'Freezing'    if temp < 32
  return 'Brr'         if temp < 60
  return 'Comfortable' if temp < 80
  'Too hot'
end

This is typical guard clause style, which both I personally and the Ruby community in general use quite often.

def temperature_message(temp)
  case
  when temp < 32
    'Freezing'
  when temp < 60
    'Brr'
  when temp < 80
    'Comfortable'
  else
    'Too hot'
  end
end

This is a typical switch as you would find it in some less powerful languages. This is probably one that I would not use, I would refactor it like this:

def temperature_message(temp)
  case temp
  when (-1.0/0.0)...32
    'Freezing'
  when 32...60
    'Brr'
  when 60...80
    'Comfortable'
  else
    'Too hot'
  end
end

Although I must admit I still find the first one easiest to read.

Since this is basically a mapping table, I would try to format it as such, so that everybody who reads the code, immediately sees the "table-ness":

def temperature_message(temp)
  case temp
  when (-1.0/0.0)...32 then 'Freezing'
  when         32...60 then 'Brr'
  when         60...80 then 'Comfortable'
                      else 'Too hot'
  end
end

This also applies to your original Java implementation:

public String getTemperatureMessage(double temp) {
    if(temp < 32) return "Freezing";
    if(temp < 60) return "Brr";
    if(temp < 80) return "Comfortable";
    else          return "Too hot";
}

Of course, since it is basically a mapping table, you might just as well implement it as a map:

def temperature_message(temp)
  {
    (-1.0/0.0)...32       => 'Freezing',
            32...60       => 'Brr',
            60...80       => 'Comfortable',
            80..(1.0/0.0) => 'Too hot'
  }.detect {|range, _| range.include?(temp) }.last
end
Jörg W Mittag