views:

469

answers:

16

This is a matter of style I've considered for a while, and I'm curious of others thoughts. The two pieces of code below are logically equivalent, which in your opinion is better style and why?:

 // no else
some_function():
   if ( my_var == 0 ) then:
       return true

   print "hello: " + my_var
   return false


// with else
some_function():
    if ( my_var == 0 ) then:
        return true
    else:
        print "hello: " + my_var
        return false
+10  A: 

I use the first one, the second one seems too verbose for me, and you avoid one level of identation which is always nice.

AlbertEin
+5  A: 

I generally prefer without the else - there's less nesting, and it reinforces the fact that the first branch will exit the loop (i.e. "there's nothing more to see here").

On the other hand, there's more symmetry with the else...

Jon Skeet
+5  A: 

I definitely prefer the later as it reads more naturally... however, endorsing that kind of statements clearly require that we all accept responsibility when the world runs out of else statements!

kasperjj
+1  A: 

I like the first one, but only when the then clause is one or two lines, and it's reasonably near the top of the function... or between two semantically distinct parts of code, in which it's quite probable the function should be split in two.

Javier
+1  A: 

Else would be a superfluous statement, which might end up embarrassing some of us when someone asks: "Why did you put that in there? You don't need it!"

le dorfier
+3  A: 

I consider this an expression of input validation. The else clause represents the valid case and is not a "alternative" execution path. If this were in the main body of a routine, you would normally write it as:

   if (my_var != 0) then:
       ... do what you need to 
   end

Since what you are basically doing is short-circuiting the function on invalid input, I would prefer the expression without the else. This leaves the main body of the function as clearly the normal case with the if statement just doing return on invalid inputs. I also like it because it reduces the indent level of the main action of the function.

tvanfosson
A: 

generally I prefer the first:
But for your example you could also rewrite it like this:

   some_function():   
       if (my_var != 0) then:       
         print "hello: " 
       return (my_var == 0);
Charles Bretana
+3  A: 

Without else. This is a guard clause and in a typical application you would see so many of these kind of statements that you get used to them.

mmiika
+1  A: 

I generally prefer:

  some_function():   
     boolean varIsZero= my_var==0;

     if (!varIsZero) then:  
         print "hello: " 

     return(varIsZero);

I realize some people don't mind a bunch of returns scattered through their code, but I try to avoid it where I think there's a more readable alternative.

Nerdfest
I agree- it is best practice to use fewer return statements
Klathzazt
The trend seems to be moving away from that, but it's coupled with smaller methods as well. As with many things, it's a matter of finding a balance that gives the best readability. I lean more towards single return where possible as it's easier to debug where the exit of the method is easily found.
Nerdfest
+7  A: 

I'll use the first form when the "if" is meant as a guard assertion at the beginning of the method. It separates the "do we really want to be here" code from the main body of the method, making it easier to understand the rest of the method at a glance.

I'll use the second form when the "if" is part of the method's logic, indicating that the method has completed.

It's purely an organizational preference though. Whatever makes the code easier to read and understand is preferred.

Terry Wilcox
It's refreshing to see so much thought and effort go into developing a personal coding style ... I'll bet your code is as sensible and easy to follow as your answer!
Adam Liss
A: 

I agree with Nerdfest's answer.. But I would do a trade-off based on the complexity of the method. If I have, say, some 50 odd lines after the initial if block, I would rather let my code return immediately.

Vijay Dev
+1  A: 

If you language has a ternary operator, this is always best:

return condition ? valueIfTrue : valueIfFalse;

Failing that, I say that it depends on the context. If the "if(foo) return" is at the top of the method, that's obvious enough, or if the second return follow immediately, then there's very little chance of confusion. e.g.,

someFunction():
 if(foo) then:
   return bar
 ... do something else
 return baz

is fine. Anyone reading it should catch the if-return at the top. However this:

someFunction():
  doSomething()
  somethingElse();
  anotherThing = 1 + 2
  if(anotherThing == 3) then:
     return 4
  doSomething()
  if(blah) then:
    tryAgain()
  etc()
  return 5

is potentially confusing because some readers will miss the return in the middle on the first pass.

noah
A: 

I'd say without the else, but whatever you decide, I'd recommend getting a tool such as ReSharper to standardize the format of your code so you never need to re-make this decision with any other developers on your team. Removing redundant else statements is one of the things it can do for you automatically.

Greg
+1  A: 

The first is more concise. I would go with that method rather than adding keywords that really aren't necessary.

Chris Ballance
+3  A: 

I try hard to have exits from a function either at the beginning (guard clauses) or at the end. So if your example really is doing a precondition check:

return true if my_var == 0
print "hello #{my_var}"
false

Basic CS "rules" say that having exit points in the middle of a function/method makes your code harder to read and understand. A reader scanning for exit points may miss the inner one(s).

Another reason I wouldn't use a full on if/else here is to be able to reduce the level of indentation.

Pistos
+1  A: 

For me it depends how "surprising" the return is.

There are certain cases - validating input parameters, checking that you're in the right state for the operation to be possible, where early exit is normal and expected. I wouldn't use an else clause for those.

But if you're half way through the method and want to exit early, the code will sometimes be easier to read with an if/else, since it makes it abundantly clear that you are dividing the world into two different cases, and doing different things in each case.

My rule of thumb would be that if it's basically arbitrary which case is the early exit, and which exits at the end of the routine, then the early exit is probably "surprising". Otherwise it may be "natural". So for instance the following two bits of code look structurally different even though they're equivalent:

if (no_can_do) return false;
do;
things;
return true;
// ------------------------
if (!no_can_do) {
    do;
    things;
    return true;
}
return false;

The second snippet is clearly evil, because it sends you off reading the end of the function for no good reason. So the early exit in the first snippet is not surprising, it's natural.

The following two bits of code look basically the same.

if (case1) {
    do;
    things;
    return true;
}
do;
different things;
return false;
// ------------------------
if (!case1) {
    do;
    different things;
    return false;
}
do;
things;
return true;

Because it's arbitrary which case is the early exit, the early exit is therefore surprising. The nature of the function, that it handles two non-trivial cases, would perhaps better be highlighted by doing:

if (case1) {
    do;
    things;
    return true;
} else {
    do;
    different things;
    return false;
}

instead. You might even write two separate routines here, one to handle case1 and the other to handle the rest, and just call one or the other.

Steve Jessop