Today I have joined a discussion that emerged on our development team: whether to use multiple return statements within the same method.
Well, back when I was a terrible newbie programmer using Clipper as the main development language, someone convinced me that every function (that was the name for a procedural equivalent of today’s object’s methods) needed one unique return point. I stickied to it but never questioned myself ever again if this is really a good thing to do. Nowadays I got myself doing just the opposite: using multiple return points and when I analyzed it, I found that really better. I’ll explain.
Imagine the following method that searches for a particular object within an ArrayList:
private boolean userExists(String name) {
boolean found = false;
for (User u : this.getAllUsers()) {
if (u != null && name.equals(u.getName())) {
found = true;
break;
}
}
return found;
}
Doesn’t this method seems a lot cleaner and easier to understand than:
private boolean userExists(String name) {
for (User u : this.getAllUsers()) {
if (u != null && name.equals(u.getName())) {
return true;
}
}
return false;
}
There are other cases where I can see the code begging for an immediate return
like:
private void loadUsers() {
if (usersLoaded) {
return;
}
// do all the lengthy user loading...
(...)
}
In my opinion, the early return when the users are already loaded are easier to interpret and easy to review by another coder than:
private void loadUsers() {
if (!usersLoaded) {
// do all the lengthy user loading...
(...)
}
return;
}
I really don’t think it’s a major problem and using one or the other is pretty acceptable but I’d like to hear from you what you think about it so please leave a comment about it if you can. Thanks.
Update: A friend of mine just e-mailed me an article that addresses the same issue.
August 15, 2006 at 8:42 pm
Felipe, encontrei esse link onde tem mais argumentos a favor do que você esta falando.
http://onthethought.blogspot.com/2004/12/multiple-return-statements.html
August 15, 2006 at 10:50 pm
Hi Felipe,
I would agree with your point, multiple return points in this case make the method more readable. There is just one thing that puzzles me: why do you check for a
nulluser? IfgetAllUsers()ever returns anulluser, I would say this is a bug: who the heck isnull? Cannullever log in?Rewriting the method without this check makes it read almost like plain english:
private boolean userExists(String name) {
for (User u : getAllUsers()) {
if (u.getName().equals(name)) {
return true;
}
}
return false;
}
Wouldn’t you agree?
August 15, 2006 at 10:51 pm
… if only wordpress wasn’t messing up indentation in the comments!
August 16, 2006 at 2:03 am
Olivier,
Regarding your question:
You are right, getAllUsers() shouldn’t return a null item at all. And
better yet, if getAllUsers() has a null user, it is desirable that a
NullPointerException is thrown there. I wrote the null check without
thinking about it
Regarding that code, there is an additional tip that I don’t know if
you know yet.
Notice that in my code I wrote:
if (u != null && name.equals(u.getName())) {
return true;
}
Taking off the “u != null” part, I have used name.equals(u.getName()).
This is a suberb protection for users with null name. Notice that the
“name” parameter is not likely to be null, so you are better off
doing:
name.equals(u.getName())
than
u.getName().equals(name)
because if u.getName() is null, the first comparation will return
false and the second will raise an exception.
Hope this helps you somehow and thanks again for your comments on my
post. That’s what make posting on a blog worth.
Best regards,
August 16, 2006 at 8:03 am
I think it’s more a matter of good sense….complex functions with one return statment might be hard to understand, because they bring a lot of code to manage the single return statment. But help to maintain pre and post function checking, debugging entry point, etc…
On the opposite, using too many return statment might break the control flow of an app, increase the difficulty to maintain..By using mutliploe return statment, people usually tend to increase the content of a single procedure, and so the complexity. The worst is when an app duplicate some lines of code because there is several entry point….
My recommendation is the following:
- try to maintain a single return statment
- unless it create “more ” (and this the difficulty) complexity than several return
So in other word, keep the single return as a recommendation, but not as an obligation…..
August 16, 2006 at 10:46 am
Well Tom, first of all, thanks for your comments. I tend to agree with you regarding that each case must be analyzed for the better approach: single or multiple returns.
However, I disagree that using multiple return increases the difficulty to maintain and the content/complexity of a procedure. What I found is that even in complex code, the return statement makes it clear because when another user is evaluating the code and reaches for a return statement it doesn’t goes on with the evaluation thinking about something like “oh! there was an if up there so this else is evaluated when…”.
Anyway, like I said, it’s a major problem
August 16, 2006 at 11:27 am
Last sentence should be:
“Anyway, like I said, it’s *NOT* a major problem”
August 16, 2006 at 1:35 pm
Felipe,
yes, your code won’t blow up when it encounters a user named
null, but I wrote the test the other way because:nullis not a valid name so throwing an exception is ok,it makes the code more english-like: “if the username is the one we are looking for” rather than “if the username we are looking for is the name of the user”.
At this point we’re discussing cosmetics!
August 16, 2006 at 4:44 pm
Oliver,
Yes, I think that maybe those issues are just cosmetics but anyway I like to analyze whether I want or not to have an Exception thrown in the method I am creating.
Sometimes protecting for nulls does more harm than good. Sometimes it masks errors that should be thrown.
Well, well maybe that’s just “much ado about nothing”