Is using an if() ... return an accepted practise ?

Question

Is using an if coupled with an immediate return like in the example below an acceptable practise instead of having a if with a block of code inside {} ? Are these equivalent in practise or is there a disadvantage to one of the approaches ?

An example in Java:

protected void doGet(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException {

ServletContext sc = this.getServletContext();       

// Throw exception for fatal error (Servlet not defined in web.xml ?)
if( sc == null )
 return; // old-style programming
 // Careful with silent bugs ! Correct way of handling this is:
 // throw new RuntimeException( "BookDetail: ServletContext is null" );

BookList bookList = WebUtil.getBookList( sc );

Answer 1

That is not a return, it's an exception. The code is perfectly ok tho.

Even if you'd replace that throw with a "return something", it would still be ok.

Answer 2

I think it comes down to readability. The code should function the same either way.

I use stuff like this all the time

Function Blah() As Boolean
  If expr Then
     Return False
  End If
  Do Other work...

  Return result

End Function

Answer 3

In your example, there is no return after the if statement; you are throwing an exception. (edit: I see you have changed the code since I posted this answer).

There are purists who think that you should have only one return statement in a method (at the end of the method). There's some merit to that idea - it makes the code more clear, it makes it easier to see what can be returned for the method, and especially when you need to cleanup resources (especially in a language without garbage collection; or in Java where you need to close for example an InputStream) it's more clear and easier if you have just one return at the bottom, and do the cleanup code just before the return.

I would not have any objection against the code in your example, however.

Answer 4

Martin Fowler would favour the early return, and calls the idea a Guard Clause.

Personally, I don't like it in Java, as I prefer one return per method. However this is subjective and I may be in the minority.

I've blogged about this for and against.

Answer 5

I have a few (subjective, or not) remarks:

• I always use accolades with a if even the block contains only one line
• I don't like to have many return on one method
• I don't think that this null check is necessary. If getServletContext() returns null, then you have a much bigger problem with your webapp that should definitely be fixed. In that case, having a NullPointerException later in the code is an exceptional error so I wouldn't bother handling it.

Answer 6

As long as you're using them for conditional escapes as the first thing in the routine. I think the fact that they are obvious in that location, and avoid at least one level of indentation outweighs the negative of having a multiple returns.

Answer 7

For error conditions, generally it's best to throw an exception - exception handling was invented to get rid of the manual return code style error checking in C that comprises about 30% of a C program.

However, early returns are fine - they are far more readable than adding an extra scope with curly braces.

if (!_cache.has_key(key))
    return null;

return _cache[key]

Is better than:

if (_cache_has_key(key))
{
    return _cache[key]
}
else
    return null;

And it only gets more obvious the more early returns that you add, 5 early returns beats the hell out of 5 nested if statements.

Note that I didn't return null on an error condition, it's expected that often the key won't be in the cache - but it still means the caller has to write code to check the result. In .NET there's a better pattern of returning a boolean, and setting the result via an out parameter. The methods beginning with Try usually follow this pattern:

Foo foo;
if (!TryGetCachedFoo("myfoo", foo))
{
    foo = new Foo(...);
    AddToCache("myfoo", foo);
}

// do something with foo