Coding best practices: thinking about it

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.

Advertisements

9 Responses to “Coding best practices: thinking about it”

  1. Marcio Says:

    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

  2. Olivier Ansaldi Says:

    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 null user? If getAllUsers() ever returns a null user, I would say this is a bug: who the heck is null? Can null ever 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?

  3. Olivier Ansaldi Says:

    … if only wordpress wasn’t messing up indentation in the comments!

  4. fcoury Says:

    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,

  5. Thomas Landspurg Says:

    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…..

  6. fcoury Says:

    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 πŸ™‚

  7. fcoury Says:

    Last sentence should be:

    “Anyway, like I said, it’s *NOT* a major problem”

    πŸ™‚

  8. Olivier Ansaldi Says:

    Felipe,
    yes, your code won’t blow up when it encounters a user named null, but I wrote the test the other way because:

    null is 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! πŸ˜‰

  9. fcoury Says:

    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” πŸ™‚

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s


%d bloggers like this: