Should a method have only one return point/statement?
Hi,
sometimes I have Code that looks like
publicboolean equals(final Object o){
if (o ==null){
returnfalse;
}
if (o ==this){
returntrue;
}
if (!getClass().equals(o.getClass())){
returnfalse;
}
if (this.getId() == 0){
returnfalse;
}
final Link cObject = (Link) o;
if (this.id != cObject.id){
returnfalse;
}
if (!this.linkParameter.equals(cObject.linkParameter)){
returnfalse;
}
if (!this.uriType.equals(cObject.uriType)){
returnfalse;
}
returntrue;
}
Is it best practice to use return in this manner? Some code metric tools report this use of return as bad coding practice.
What are the pros and cons of the short return?
Thanks for your answers
# 1
If possible you should have only one return point from your method. This makes it easier for people to analyze the program execution. This is especially useful if you have many exceptions being caught in the method mixed with final statements.
Its not always possible to stick with this rule but I would suggest that whenever you write code that does not you probably want to examine it long and hard to see if there is a better way of doing it.
matfud
# 2
I would say that if your code is readily expressed with multiple returns, go for it.
Sometimes it is natural to think of a function as:
"If the caller gave us this bogus argument, complain and return.
If this, then do this, and that's it.
If so-and-so, then we need to do this, and we are done.
Otherwise, we are on the Happy Path, so do the rest until the closing }."
If that is a natural way to think of your method, then make the code reflect it. If you jump through convoluted hoops and make your code a twisty maze of deeply nested if/else blocks to bow to some theoretical "one-return-only" rule, the guy who has to maintain your code after you will show up at your door one dark and stormy night, wielding something heavy and sharp.
(Cue ......u......j.....)
# 3
A good tip when writing equals methods is to set your return result to false at the start and then try to prove that the two objects are equal. For example the code posted earlier can be rewritten as:
public boolean equals(final Object o ){
boolean result = false;
if ( getID() != 0 &&
null != o &&
getClass() == o.getCass()) {
Link link = (Link) o;
if (getID() == link.getID() &&
linkParameter.equals(link.linkParameter) &&
uriType.equals(link.uriType)) {
result = true;
}
}
return result;
}
Not necessarily any nicer but it starts off assuming that the objects are not the same and then tries to prove they are. This often results in clearer code (though many would disagree :)
matfud
# 4
It is perhaps instructive to consider the "one return statement" from the perspective in which this bit of sage advice evolved.
Imagine, if you can, that you are writing FORTRAN code, one statement per punch card, which you submit to a grunt who will feed it into a mainframe from which you will eventually get an output listing from the line printer.
Debugging consists of punching cards that print out values of variables (most of which are kept in some global common area and are thus always visible) and slipping these cards into the deck at appropriate places.
IF you always exit every subroutine from a single return statement located at the end of the routine it is easy to slip a print statement into the end of a procedure and see what it did. If you have multiple exits, debugging is not so easy.
Of equal importance, or maybe even of more importance, if you use Bob's SVD routine, and Phil's Chart output subroutines, which means borrowing their decks of cards and running them through with yours, you will find your ability to debug substantially degraded if you must read through Bob's code to see if he properly used a single exit. You want to KNOW that everyone in your shop followed the style mandates and didn't ever exit from the middle of a routine. It was especially obscene to shove in a RETURN statement when it was just as easy to GO TO to the single numbered return statement at the end.
The modern world is somewhat different. The structured programming crowd effectively eliminated the convenience of jumping to the one single return statement and symbolic debugging pretty much eliminated the necessity of routinely locating the exit of a routine to facilitate stuffing code with temporary print statements.
As a result some "stylistic" things that were not only best practice of the day but also mandated in some shops, have ceased to have the importance that they once did.
Style rules often derive most of their meirt from enforcement rather that from anything intrinsic to them. They resemble driving on the right side of the road. Which side does not matter in the least, but enforcement allows all the people on the road to make assumptions (correct assumptions!) about what the other drivers will do.
You would be correct in crucifying anyone whose code violated the first letter capitalized for class names and lower case for members and proceedures style convention. Such code is not fit to read. But the ONLY reason that it is unfit to read is because that style is so widely practiced that you can almost depend on it to instantly tell what you are looking at.
Because the "single exit from a subroutine" does NOT currently enjoy that nearly universal acceptance it's merit as a useful rule of style is significantly reduced. While it might be useful for me to know that all the routines that I will read exit from a single point, I can't count on it because that isn't how everyone does things. Sort of a catch 22, the only conventions that matter are the ones that matter.
# 5
> Style rules often derive most of their meirt from enforcement rather
> that from anything intrinsic to them. They resemble driving on the right
> side of the road. Which side does not matter in the least [ ... ]
Riding a horse on the right side of the road in the dark ages made it
nearly impossible for right handed sword fighters to combat each
other when they were passing each other. It's the 'peaceful' side.
kind regards,
Jos (left handed ;-)
JosAHa at 2007-7-28 17:22:18 >
# 6
In addition to what has already been said here, there is another more current reason for using this technique. In languages with manual memory control, you need to make sure that you deallocate locally allocated memory before returning.
Using multiple return points can in these cases introduce memory leaks. For instance:
void foo(int memsize) {
char *buf = new char[memsize]; //dynamic memory
boolean succeeded = doSomething(buf);
if (!succeeded) {
return; //memory leak
} else {
doSomethingElse(buf);
}
delete[] buf; //proper resource deallocation
}
Since java has a GC this is not really a problem, and personnaly I think that multiple return points makes the code more readable.
# 7
A agree with putting
boolean result = false;
at the top of the if/else if statement. However, equally valid is to have
boolean result = true;
at the top, and negating all the logic in the if/else statements. Which to use depends on if negating the logic makes it easier to read or harder. If both are about as easy to read, I perfer to use the affirmative argument:
boolean result = true;
And to add a bit of nit-picking, I would prefer
boolean isEqual = true
to this
boolean result = ture.
# 8
"Riding a horse on the right side of the road in the dark ages made it
nearly impossible for right handed sword fighters to combat each
other when they were passing each other. It's the 'peaceful' side."
And having the steering wheel on the left side makes it easier to hit the kids in the back seat. Just thought I'll clear that up.
# 9
Forcing code to have a single return point is not only pointless in Java it can prevent the compiler from pointing out problems in your code.
It's completely uncecessary because Java has finally blocks.
How it defeats the compiler is easy to demonstrate than explain. Consider the following real code I found recently:
public ActionForward execute(ActionMapping mapping, ActionForm form, HttpServletRequest request,
HttpServletResponse response) throws Exception
{
HttpSession session = request.getSession(false);
ActionForward forward = new ActionForward();
if (session != null)
{
session.removeAttribute(Constants.USER_KEY);
session.removeAttribute(Constants.ADMIN_ROLE);
forward = mapping.findForward(Constants.SUCCESS);
}
return forward
}
Do you see the problem? If session is null, then the forward is never assigned to a meaningful value. It's a hard situation to produce but if that were the case, it bascially bombs on the user.
Now, lets get rid of the forced single-return.
public ActionForward execute(ActionMapping mapping, ActionForm form, HttpServletRequest request,
HttpServletResponse response) throws Exception
{
HttpSession session = request.getSession(false);
if (session != null)
{
session.removeAttribute(Constants.USER_KEY);
session.removeAttribute(Constants.ADMIN_ROLE);
return mapping.findForward(Constants.SUCCESS);
}
}
Now when you compile (or as you are working on it in any decent IDE) you'll get an error. The compiler will tell you 'hey *******, not all paths in your code return a value!'.
I don't see any value in forcing the single return. Actually I think it generally makes code much harder to read. It turns the execution path from a tree into an onion.
Sometimes, of course it will make sense to have a single return. It wll be easier to write and easier to understand. There's a related issue I see in code when this is the case. Unecessarily assigning defaults to local variables. If you just declare the variable and don't assign it, the compiler will complain if there is a path that doesn't guarantee that it is assigned before it is used. You can't always avoid assigning a default but you should as much as possible and you can often restructure the method to allow this and make it much clearer.
# 10
> If possible you should have only one return point
> from your method. This makes it easier for people to
> analyze the program execution. This is especially
> useful if you have many exceptions being caught in
> the method mixed with final statements.
I disagree completely. It usually makes it much harder to figure out what the program returns and when. All a single return tells you is that the method has a single return. It often obscures meaning. I've often had to untangle such code just to verfiy that it is working properly.
# 11
> If that is a natural way to think of your method,
> then make the code reflect it. If you jump through
> convoluted hoops and make your code a twisty maze of
> deeply nested if/else blocks to bow to some
> theoretical "one-return-only" rule, the guy who has
> to maintain your code after you will show up at your
> door one dark and stormy night, wielding something
> heavy and sharp.
The 'one return' rule is actually a good one, if you are coding in COBOL or some nightmare language that allows gotos and all other kinds of booby-traps.
The problem is that a lot of people missed the memo about how languages like Java enforce the rules of structured programming implicitly. There's no reason to enforce structured programming rules over top of the compiler. This is 2007, not 1982.
