5

Possible Duplicate:
Elegant ways to handle if(if else) else

In a function where there are multiple returns, what is the best style to use?

This style, where there is no 'else' because it is frivolous and you think it is more concise?

f() {
    if (condition) {
        return value;
    }

    return other_value;
}

Or this, if you think that 'else' provides nice syntax sugar to make it more readable?

f() {
    if (condition) {
        return value;
    } else {
        return other_value;
    }
}

Feel free to share whatever other reasons you may have for preferring one over another.

EDIT:

The above samples are hypothetical, if they were this simple, as Yannis Rizos noted below, the ternary operator ( condition ? value : other ) would be acceptable. However I'm also interested in cases like this:

f() {
    if (condition1) {
        return value;
    } else if (condition2) {
        return value2;
    }
    ...
    else {
        return other_value;
    }
}
Community
  • 1
Thomas Dignan
  • 684

5 Answers5

7

I prefer this one: 

return (condition) ? value : other_value;

but it's just a matter of personal preference. On any team I've worked I was happy to comply with whatever the code convention document preferred.

"Readability" is too vague a term to apply in such a specific and small issue and there's no technical reason to prefer one style over the others. Whichever one you feel is more readable, what matters most is to be consistent.

This StackOverflow question on Improving Code Readability gives some great points on other, more important, aspects of readability you should be concentrating on instead.

I got that the question was a hypothetical, but my answer still stands. It really doesn't matter.

On the expanded example, I prefer: 

f() {
    if (condition1) {
        return value;
    } else if (condition2) {
        return value2;
    }
    ...

    return other_value;
}

But for a very specific reason: I organize the blocks in such a way that the last one, the return other_value; is the one that's more likely to happen, so my code hints to business logic - and I'm not pretending that's smart. Still a strictly personal preference, the only reason behind it is my personal process of organizing code, one that I'd happily avoid if it goes against the team's conventions.

In any case, if you have more than a couple consecutive else if blocks, rewriting into a switch statement would probably make more sense, readability-wise.

Community
  • 1
yannis
  • 39,647
7

Of course, as others have pointed out it is mostly a matter of personal preference. The key word here being mostly. There are, however, some objective guidelines that can be used, and personally, I like forming my personal preferences based on objective guidelines, regardless of how insignificant these may be.

So, the way I do it is as follows:

if( condition )
    return;
do-other-stuff;

And I always code multiple-if-else statements as follows:

if( a ) return x;
if( b ) return y;
if( c ) return z;
return null; //whatever, I ran out of letters

And here is why:

Deep down at the assembly language level, the 'else' keyword corresponds to some code. An if-then-else statement in assembly language looks like this:

    test condition
    if false, jump to else
    perform the if-part
    jump to end
else:
    perform the else-part
end:

The 'jump to end' instruction can be thought of as the code that the 'else' part corresponds to. Now, consider how the above code fragment changes when the if-part is a return statement:

    test condition
    if false, jump to else
    do-some-stuff-and-return
    jump to end
else:
    perform the else-part
end:

Clearly, the 'jump to end' instruction will never be executed. I would be willing to bet that when a compiler analyzes your code to find unreachable code, it does figure out that the 'else' keyword is unreachable, and it factors it out, but it refrains from issuing a warning because this style is used by many out there, and it does not want to upset them. But I use this as an objective justification, no matter how insignificant it is, for never using the 'else' keyword when the if-part returns.

Mike Nakis
  • 32,803
3

I like what Uncle Bob wrote in Clean Code. It was something along the lines that when you write clean and readable code, your functions should be small enough (and obviously fit on a single page) that you shouldn't need these type of guidelines or style specifications.

Instead your #1 objective is to improve readability as much as you can. As long as its readable and clear, anything else is just a personal preference. I'm assuming the example you supplied was just a hypothetical example but if it was actual code, then Yannis definitely gave you a more readable alternative.

DXM
  • 20,022
2

My personal preference is to include the unnecessary else. I find that I can grok the control flow at a glance when it's there, versus having to remember to hunt for returns that branch the flow implicitly. A couple times I've come back to cold code and added something after an "if" and been confused about why it wasn't running, only to realize I hadn't seen the return that bailed the function before getting to my new stuff.

Dan Ray
  • 9,116
  • 3
  • 38
  • 49
1

The "single var pattern" often found in Javascript suggests single points of return by insisting that all variables are declared at the top of the function body to prevent "hoisting". Thus, recording the evaluations instead of returning can prevent the need for an ELSE (which I have a strange aversion to...):

 function doThis(required){
    var result = false;
    if(required){
        //base implementation
        result = doThat();
    }
    return result;
 }

If you have sequential or optional conditions and can't break out to another function, then you can do this:

 function doThis(required, opt){
    var result = false,
        oRequirement = 'foo';
    if(required){
        //base implementation
        result = doThat();
        if(result !== false && opt !== undefined && opt === oRequirement){
                //optional implementations
                result = doOptional();
            } else {
                //undo previous truthiness
                result = false;
            }
        }
    }
    return result;
 }

You could delegate consumption of variables externally, making this function effectively a "guard clause":

 function vA(a){
     return true;
 }

 function vB(c){
     return true;
 }

 function vB(c){
     return true;
 }

 function doThis(required, opt){
    var result = false;
    if(vA(required)){
        //base implementation
        result = doThat(required);
        if(vB(result) && vC(opt)){
            //optional
            result = doOptional(opt);
        }else{
            result = false;    
        }
    }
    return result;
 }

You could layer your evaluations:

 function doThis(required, opt){
    var result = false;
    if(if(vA(required)){
      //base
      result = doThat(required);
    }

    if(if(vB(result) && vC(opt)){
      //optional
      result = doOptional(opt);
    }
    return result;
 }

Ternaries can reduce size, and are very pretty, but they can be a little harder to debug, and a bit inconvenient if you need much in the way of nested evaluation steps:

function doThis(a, b, c){
    var o = false;
    if(valid(a)){ //assuming some sort of validation is required
        if(b !== undefined && valid(b)){
            o = c ? //an optional step
                doMore(a) ?
                    new Foo(a, b) :
                    false :
                new Bar(b, a) ;
        }
    }
    return o;
}

Generally speaking, I need to nest IF statements if optional arguments are present. When the outcome is the result of a process within the body, then it suggests delegating or chaining into a separate method.

Having said that, I can't help but feel that there is no "right" way (although there are certainly wrong ones...), and it's this fact that makes coding a creative profession capable of a broad range of expression. I mean, who doesn't love a bit of refactoring?

sunwukung
  • 2,275