Thursday, April 26, 2007

Guard Clauses Considered Harmful

A colleague and I have had a long running debate on Guard Clauses, which is an example of "Alligators versus Crocodiles". A previous post tries to present the "for" case.

Now, it's hammer time.

IMO, the canonical form for a Java method is like so:


public MyObject doSomething(Object a, Object b) {
    MyObject result = null;

    // stuff goes here

    return result;
}


It's old-school: a single point of return. Any checks should concentrate on the positive and relegate error messages, exceptions, etc to the else statements at the bottom of the method.

For example:


if( a != null ) {
    if( b != null ) {
        doSomething();
    } else {
        // throw exception
    }
} else {
    // log error
}


Here are the advantages:

  • The logic of the method is easy to understand: set the default case first, then change it if necessary. It quickly becomes a pattern that makes reading and debugging code easier.
  • It is easy to enforce. There is no slippery slope as with guard clauses. Guard clauses easily degenerate into "open season" for returning from methods all over the place. This is much harder to maintain, and is more prone to bugs, than the (admitted) indentation issue. IDEs can help match braces etc with respect to indentation: may heaven help you if you need to understand a method with several return methods. Note that the example of guard clauses (here) is pristine. It's not always this way.
  • Stating the above in a different way, it's easy to scan and find the "heart" of the logic. The corner cases of exceptions and error logging is at the bottom of the method. Guard clauses puts that stuff right up front, which strikes me as being inverted.
  • Using a convention is extremely powerful: note that methods that use this format are amenable to being processed by simple scripts etc. e.g. If one wanted to insert "pre"/"post" logic logging (though admittedly this is surpassed by using AOP).
Counters to the "for" case:
  • In the purest form, guard clauses are brief statements at the top of the method and, yes, they can be thought of as a pseudo-contract. But when scanning through the method, there is something jarring about seeing a return statement. It invariably opens another thread in my head for that case, and as the returns stack up, so too do these threads. It is distracting.
  • An oft-cited argument for guard clauses is that methods in Java are shorter. Hmmm.... maybe. Example methods are shorter, and the pristine methods you and I write are shorter, but does the average Java Joe/Jane really write shorter methods than, say, C++? IMO, the above convention does not harm short methods but definitely assists the reader with the dreaded longer methods. The proper use of guard clauses is just too subtle for Java Joe/Jane.
And there you have it.... As always, I (and my colleague, who is no doubt reading closely) am interested in your feedback.

Do you see guard clauses as an alligator or a crocodile?

ps. I fully concede that this convention doesn't have a sexy term like guard clause and does not get press in popular books. Perhaps fans of this idea can give it a snazzy name (Return on Rails?)

9 comments:

Dale said...

Mike says that he picked up the single-point-of-return coding style from me while working on DRA Web2. I should mention where I got it:

As a long time C/C++ programmer I used to return,break,continue at will (although I drew the line at goto!) Then I started working on early versions of windows.

Windows coding tends to be event/message driven. A general pattern is:
HandleSomeEvent()
{
allocate resources
(i.e. brushes, etc)
do your thing
free resources
return
}

However "allocate resources" can fail and you *MUST* *NOT* leak GDI resources, so this turns into:

if(!allocate resource 1)
return
if(!allocate resource 2)
free resource 1
return
do your thing
free resource 2
free resource 1

If you extend this technique to add resource 3, 4, etc. you can see it gets very messy, very redundant, and very hard to maintain.

This was the motivator for the style:

if(allocate resource 1)
if(allocate resource 2)
do your thing
free resource 2
else
complain about resource 2
free resource 1
else
complain about resource 1
return

which has all the benefits that Mike talks about in his blog entry. Bottom line is when I adopted this programming style and convinced (or coerced) my co-workers into using it too, the bug count/leak count dropped dramatically.

However, times and coding styles have changed. The most significant change is the RAII pattern. Just as the advent of type-checking compilers made hungarian notation obsolete (and severely counterproductive!) RAII makes single-point-of-return much less compelling.

I still avoid returns except in very well defined cases (like guard clauses), but I'm not as adamant about it as I used to be. [ sorry Mike ;-) ]

M Easter said...

Thanks for the good comment, Dale.

I definitely agree that one has to evolve and think about the rationale for things. Long ago, I tried to use a "m_" prefix for class members in Java for about 2 weeks before I re-evaluated and jettisoned it (because of the auto-generation of getters/setters in IDEs).

It has been a long time since I worked with C++. For readers who, like me, need a refresher on RAII, here it is.

Note that there is a bit of mismatch here: I work in Java, where life is quite a bit different than C++. The above link talks about how the RAII pattern and GC/finally blocks etc.

Though I have lost a key supporter (zounds!), I stand by the idiom -- not by reflex, but at the very least by aesthetic.

Interestingly, my new client site uses multiple returns with abandon. We'll see who gets converted because something will have to give.

Anonymous said...

The noose is tightening.

I feel the grasping... the desperation... Oxygen deprivation is beginning to set in.

Won't someone please come to Mike's rescue?

BTW, I have never recommended using multiple returns "with abandon" as you are currently experiencing -- only within the well-defined idiom of guard clauses.

Also, with the test-first mantra of "Red, Green, REFACTOR", if you are seeing so many guard clauses that it truly distracts from the mainline of the code, I would expect that to be a code smell: you need to get too many ducks in a row in order to safely call your method. That becomes a "contract" that is hard to enforce, and out to be refactored -- perhaps into a validateInputs() method or something like that.

And I agree with Dale that I would be MUCH less apt to use guard clauses in C/C++.

But you are still GOING DOWN...

Bwah, hah, hah ;->

M Easter said...

re: anonymous post. I see my old compadre has chosen to enter the fray.

I concede that I don't have many supporters here, but it ain't over yet :-)

re: multiple returns with reckless abandon. Of course, no one espouses that but that is the danger of guard clauses, IMHO. It invites people to "degenerate" code into the wild west, where multiple returns drift like tumbleweeds. I'm thinking of Dave Thomas' "Broken Windows" analogy here. Remember, Kevin, you won't be there when some college kid modifies your method. And s/he is going to see those tasty return statements.

Even if you are there, I can easily imagine time wasted on whether or not a given return statement _is_ a guard clause or not. Life is just a lot easier with a single point of return.

Anonymous said...
This comment has been removed by a blog administrator.
Anonymous said...
This comment has been removed by a blog administrator.
crowder said...

Code that makes use of guard-clauses necessarily has lower cyclomatic complexity than code that doesn't.

Code that makes use of guard-clauses is necessarily more performant than code that doesn't.

If you're worried about leaking resources, RAII will save you, one-return-per-function will not. It is as easy to update a "return_value" variable and forget to release it's previously assigned, allocated value, as it is to return without releasing it. This is why smart_ptr<> and friends exist.

J. B. Rainsberger said...

Mostly circular reasoning. "Guard Clauses are bad because, well, they're bad!" With short methods, multiple return points don't hurt. Why do you find returns jarring? Long methods. Don't write long methods; problem solved.

PS: Guard Clauses encourage separating jumbled responsibilities by making the jumble of responsibilities more explicit. Step 1: Guard Clause; Step 2: Separate responsibilities; Step 3: Profit.

Anonymous said...

I think that the debate is endless when talking about guard clauses .

In my opinion they are very good because improve the readability of your code which makes them easier to read therefore easier to maintain.

The concept behind guard clauses is very simple yet very powerful and clear and is:
"If the method doesn't receive the parameters in a valid state, don't execute it, just leave".

Just ask to yourself, why bothering with a bunch of if/else clauses when you can just avoid executing something at all if the required data of the method is not valid?.


Adding Guard clauses may look like a burden if you have only one if/else clause or two (and yet still are better than them) but you will realize the power of them after four five of them.

Another way to see this is that a method should be responsible of only one thing, one action, if you are having code like the one on your post, then you have a code smell that means that your method is being responsible of more than one thing and that is why you need that bunch of ifs.