views:

434

answers:

15

Some people consider multiple return statements as bad programming style. While this is true for larger methods, I'm not sure if it is acceptable for short ones. But there is another question: Should else explicitly be written, if there is a return statement in the previous if?

Implicit else:

private String resolveViewName(Viewable viewable) {
    if(viewable.isTemplateNameAbsolute())
        return viewable.getTemplateName();
    return uriInfo.getMatchedResources().get(0).getClass().toString();
}

Explicit else:

private String resolveViewName(Viewable viewable) {
    if(viewable.isTemplateNameAbsolute())
        return viewable.getTemplateName();
    else
        return uriInfo.getMatchedResources().get(0).getClass().toString();
}

Technically else is not necessary here, but it make the sense more obvious.

And perhaps the cleanest approach with a single return:

private String resolveViewName(Viewable viewable) {
    String templateName;
    if(viewable.isTemplateNameAbsolute())
        templateName = viewable.getTemplateName();
    else
        templateName = uriInfo.getMatchedResources().get(0).getClass().toString();
    return templateName;
}

Which one would you prefer? Other suggestions?

+17  A: 

Other obvious suggestion: use the conditional operator.

private String resolveViewName(Viewable viewable) {
    return viewable.isTemplateNameAbsolute()
        ? viewable.getTemplateName()
        : uriInfo.getMatchedResources().get(0).getClass().toString();
}

For cases where this isn't viable, I'm almost certainly inconsistent. I wouldn't worry too much about it, to be honest - it's not the kind of thing where the readability is like to be significantly affected either way, and it's unlikely to introduce bugs.

(On the other hand, I would suggest using braces for all if blocks, even single statement ones.)

Jon Skeet
I'm gonna have to agree with always using braces (changed my answer to reflect that), though I usually let Eclipse handle that for me with autoformat + save actions.
Andrei Fierbinteanu
+1 - though I think you have an extra close paren in there.
Carl
@Carl: Fixed, thanks.
Jon Skeet
+4  A: 

I usually prefer the first option since it's the shortest.

And I think that any decent programmer should realize how it works without me having to write the else or using a single return at the end.

Plus there are cases in long methods where you might need to do something like

if(!isValid(input))  { return null; }// or 0, or false, or whatever
// a lot of code here working with input

I find it's even clearer done like this for these types of methods.

Andrei Fierbinteanu
+2  A: 

Depends on the intention. If the first return is a quick bail-out, then I'd go without the else; if OTOH it's more like a "return either this or that" scenario, then I'd use else. Also, I prefer an early return statement over endlessly nested if statements or variables that exist for the sole purpose of remembering a return value. If your logic were slightly complex, or even as it is now, I'd consider putting the two ways of generating the return value into dedicated functions, and use an if / else to call either.

tdammers
A: 

Sure, people have a lot to say about programming style, but don't be so concerned about something relatively trivial to your program's purpose.

Personally, I like to go without the else. If anybody is going through your code, chances are high he won't be too confused without the else.

Angad
A: 

I prefer the first one.

Or... you can use if return else return for equally important bifurcations, and if return return for special cases.

When you have assertions (if p==null return null) then the first one is the most clear by far. If you have equally weighted options... I find fine to use the explicit else.

helios
A: 

I prefer the second option because to me it is the quickest to read.

I would avoid the third option because it doesn't add clarity or efficiency.

The first option is fine too, but at least I would put a blank line between the first bit (the if and its indented return) and the second return statement.

In the end, it comes to down to personal preference (as so many things in programming style).

Hollance
+6  A: 

i prefer the cleanest approach with single return.To me code is readable, maintainable and not confusing.Tomorrow if you need to add some lines to the if or else block it is easy.

1.) code should never be clever.

Suresh S
+1  A: 

It's completely a matter of personal preference - I've literally gone through phases of doing all 4 of those option (including the one Jon Skeet posted) - none of them are wrong, and I've never experienced any drawbacks as a result of using either of them.

Kragen
+3  A: 

I prefer multiple returns in an if-else structure when the size of both statements is about equal, the code looks more balanced that way. For short expressions I use the ternary operator. If the code for one test is much shorter or is an exceptional case, I might use a single if with the rest of the code remaining in the method body.

I try to avoid modifying variables as much as possible, because I think that makes the code much harder to follow than multiple exits from a method.

Jörn Horstmann
+1 for the difference between equally valid code paths and exceptional guard conditions.
ILMTitan
A: 

Considering multiple return statements "bad style" is a long, long discredited fallacy. They can make the code far clearner and more maintainable than explicit return value variables. Especially in larger methods.

In your example, I'd consider the second option the cleanest because the symmetrical structure of the code reflects its semantics, and it's shorter and avoids the unnecessary variable.

Michael Borgwardt
A: 

The stuff about only one return statement dates from the 1970s when Dijkstra and Wirth were sorting out structured programming. They applied it with great success to control structures, which have now settled down according to their prescription of one entry and one exit. Fortran used to have multiple entries to a subroutine (or possibly function, sorry, about 35 years since I wrote any), and this is a feature I've never missed, indeed I don't think I ever used it.

I've never actually encountered this 'rule' as applied to methods outside academia, and I really can't see the point. You basically have to obfuscate your code considerably to obey the rule, with extra variables and so on, and there's no way you can convince me that's a good idea. Curiously enough, if you write it the natural way as per your first option the compiler usually generates the code according to the rule anyway ... so you can argue that the rule is being obeyed: just not by you ;-)

EJP
+2  A: 

Keep the lingo consistent and readable for the lowest common denominated programmer who might have to revisit the code in the future.

Its only a few extra letters to type the else, and makes no difference to anything but legibility.

borisx
+4  A: 

The "single point of exit" dogma comes from the days of Structured Programming.

In its day, structured programming was a GOOD THING, especially as an alternative to the GOTO ridden spaghetti code that was prevalent in 1960's and 1970's vintage Fortran and Cobol code. But with the popularity of languages such as Pascal, C and so on with their richer range of control structures, Structured Programming has been assimilated into mainstream programming, and certain dogmatic aspects have fallen out of favor. In particular, most developers are happy to have multiple exits from a loop or method ... provided that it makes the code easier to understand.

My personal feeling is that in this particular case, the symmetry of the second alternative makes it easiest to understand, but the first alternative is almost as readable. The last alternative strikes me as unnecessarily verbose, and the least readable.

But @Jon Skeet pointed out that there is a far more significant stylistic issue with your code; i.e. the absence of { } blocks around the 'then' and 'else' statements. To me the code should really be written like this:

private String resolveViewName(Viewable viewable) {
    if (viewable.isTemplateNameAbsolute()) {
        return viewable.getTemplateName();
    } else {
        return uriInfo.getMatchedResources().get(0).getClass().toString();
    }
}

This is not just an issue of code prettiness. There is actually a serious point to always using blocks. Consider this:

String result = "Hello"
if (i < 10) 
    result = "Goodbye";
    if (j > 10) 
         result = "Hello again";

At first glance, it looks like result will be "Hello again" if i is less than 10 and j is greater than 10. In fact, that is a misreading - we've been fooled by incorrect indentation. But if the code had been written with { } 's around the then parts, it would be clear that the indentation was wrong; e.g.

String result = "Hello"
if (i < 10) {
    result = "Goodbye";
    }
    if (j > 10) {
         result = "Hello again";
    }

As you see, the first } stands out like a sore thumb and tells us not to trust the indentation as a visual cue to what the code means.

Stephen C
A: 

as somebody said already approach with single return is this case is very and less readable. I prefer first verstion

+1  A: 

Since you asked for other suggestions, how about

Use asserts or RuntimeExceptions

    private String resolveViewName(Viewable viewable) {
    if(viewable.isTemplateNameAbsolute())
        return viewable.getTemplateName();
    else
        return uriInfo.getMatchedResources().get(0).getClass().toString();
    throw new RuntimeException ( "Unreachable Code" ) ;
    }

Use the final keyword

private String resolveViewName(Viewable viewable) {
final String templateName;
if(viewable.isTemplateNameAbsolute())
    templateName = viewable.getTemplateName();
else
    templateName = uriInfo.getMatchedResources().get(0).getClass().toString();
return templateName;
}
emory
`final` is a good addition.
deamon
Why did you add this RuntimeException?
DerMike
@DerMike I believe that because of the previous returns the RuntimeException is unreachable. If it is thrown, I will know that I was wrong.
emory
@emory I see. Yet it just lengthens an otherwise short and simple method (even contradicts itself when it is thrown... someday :-) ).
DerMike