Code guidelines for checks ?

[nicolas.raoul]

Hello all,

I am defining the guidelines for the readability of a Java project,
and I am hesitating between two ways of implementing imbricated if-
else in the case of checking conditions before doing something.

Let's say I have to check two conditions before doing something.
Here are the two kinds of implementations I have seen around in open
source projects:

=============== Implementation 1 ===============


if ( foo != null ) {
if ( bar != null ) {
doSomething ( foo, bar );
} else {
throw new NullBarException();
}
} else {
throw new NullFooException();
}


=============== Implementation 2 ===============


if ( foo == null ) {
throw new NullFooException();
}

if ( bar == null ) {
throw new NullBarException();
}

doSomething ( foo, bar );


================================================

Some drawbacks of Implementation 1:
- As the number of checks grows, the indentation becomes a problem.
- It is harder for the reader to see the cause-exception couples.

Some drawbacks of Implementation 2:
- Three blocks instead of one, so it does not look as a code entity.

Which implementation do you think is better for readability and
maintenance ?
Cheers,
Nicolas (http://nrw.free.fr)

 

[Flo 'Irian' Schaetz]

And thus spoke (e-mail address removed)...

Some drawbacks of Implementation 1:
- As the number of checks grows, the indentation becomes a problem.
- It is harder for the reader to see the cause-exception couples.

Some drawbacks of Implementation 2:
- Three blocks instead of one, so it does not look as a code entity.

Which implementation do you think is better for readability and
maintenance ?

Personally, I always use the 2nd way. It's much more easy to read,
maintain, etc. - in my (not so) humble opinion, of course. I like the
advantage of first being able to check for all possible errors (null
pointers, etc.) and then proceed with the really important code...

Flo

 

[Thomas Fritsch]

=============== Implementation 1 ===============


if ( foo != null ) {
if ( bar != null ) {
doSomething ( foo, bar );
} else {
throw new NullBarException();
}
} else {
throw new NullFooException();
}


=============== Implementation 2 ===============


if ( foo == null ) {
throw new NullFooException();
}

if ( bar == null ) {
throw new NullBarException();
}

doSomething ( foo, bar );


================================================

Some drawbacks of Implementation 1:
- As the number of checks grows, the indentation becomes a problem.
- It is harder for the reader to see the cause-exception couples.

Some drawbacks of Implementation 2:
- Three blocks instead of one, so it does not look as a code entity.

Which implementation do you think is better for readability and
maintenance ?

My vote is for Implementation 2.
I totally agree with your drawbacks of Impl 1.
I don't quite understand what you describe as the drawback of Impl 2.
The logic (both in Impl 2 and in Impl 1) is a sequence of 3 steps (two
sanity checks, and one real work-step). So why want it to appear as one?

 

[nicolas.raoul]

Thanks for your answers

I have been thinking about a serious drawback for Implementation 2:
let's say a failed condition does not lead to an exception but makes
the method return an error code. Then Implementation 2 would contain
three "return" keywords, and rumor has it that it is cleaner to have
as few "return"s as possible. Which leads me to:

=============== Implementation 3 ===============

if ( foo == null ) {
throw new NullFooException();

} else if ( bar == null ) {
throw new NullBarException();

} else {
doSomething ( foo, bar );
}

================================================

Implementation 3 is not as pretty as Implementation 2, but it makes it
possible to set a return code and only return it at the end of the
method.
A typical example can be found in the WIMPI project (seems to be
developed by Google) : http://www.koders.com/java/fid57D6AD8F77D867C8A96F08B7F88476BDA0AAF30F.aspx?s=parameter+view


Meanwhile, I have found a typical example of my scenario in method
getCommand of the Spring MVC:
http://springframework.cvs.sourcefo...FormController.java?revision=1.44&view=markup
I think that the Spring project is a modern project with strong
guidelines, so I would tend to think they kind of set up an example.
In that method, the typical scenario of checking for conditions before
doing something, they use Implementation 2.

If anyone favors Implementation 1, I would like to hear from you !

Cheers,
Nicolas RAOUL.

Thomas: You're right, it actually makes more sense to see it as
different steps rather than one big step.

 

[Lew]

Thanks for your answers

I have been thinking about a serious drawback for Implementation 2:
let's say a failed condition does not lead to an exception but makes
the method return an error code. Then Implementation 2 would contain
three "return" keywords, and rumor has it that it is cleaner to have
as few "return"s as possible. Which leads me to:

Vicious rumor. Not true.

=============== Implementation 3 ===============

if ( foo == null ) {
throw new NullFooException();

} else if ( bar == null ) {
throw new NullBarException();

} else {
doSomething ( foo, bar );
}

================================================

Implementation 3 is not as pretty as Implementation 2, but it makes it

It's identical to number 2 if you drop the "else" keywords.

But you beg the question of whether you should throw an exception for those
conditions. It's not always the Right Thing to do.

possible to set a return code and only return it at the end of the
method.

That isn't what you illustrated. You illustrated not setting a return code,
thus avoiding all the messily deep indentation that your idea would necessitate.

Readability and maintainability are not always enhanced by some of the
anal-retentive conventions people suggest. The "single-return" philosophy is
in this category.

 

[Roedy Green]

if ( foo == null ) {
throw new NullFooException();
}

if ( bar == null ) {
throw new NullBarException();
}

doSomething ( foo, bar );

I hate nesting, so I prefer this style, though I suspect I will be in
the minority.

Nesting his a sort of procrastination, where you have keep in mind all
the things you plan to do in future.