Chris said:
What I read into your post was that reducing returns could be seen as an end in
itself. And that is /precisely/ what I disagree with.
Okay, I think I see the confusion.
It's more of a way to stimilute criticism of code
to ask the question: would it be clearer to have fewer?
Just like asking would it be clearer to introduce an extra class.
I probably overstated the idea in the way I mentioned one return.
I should not have given value to just one when I meant:
Fewer is better, anyone can understand when there is only one, but don't
go out of your way to make your method have just one when two or three
oftens make the code very readable.
If I had to make a rule of thumb on this particular issue, then I'd say that
more early returns is better than fewer. I.e. I'd like to see people actively
looking for chances to code algorithms in such a way that early returns can be
used to prune the problem space, and hence the complexity of the expression.
This method of cleaning up code can often be combined with refactoring
into new methods, such that a small number of ways (two or three) to
break out of tricky bit of logic can all occur in one method reducing
the higher level to something akin to a series of simple tests.
I consider it a mistake to encourage people to avoid embedded returns.
Sorry, just saying more early returns makes better code I would
not good beginner advice either. Certainly, setting lots of flags to
carry around just to make the code fall through to its final line makes
for really poor code, but open season on returns anywhere is not without
its pitfalls. You already mentioned, more than a handfull and it's time
to consider some cleaning, but other rules might be worth stating like:
lots of simple guard conditions makes for more readable code than
appearances of multiple returns in various places. Where guard
condition is defined as: an as simple as possible conditional that may
result in a return at the highest level of code, less of a surprise and
hence more readbale when they occur near the top of a routine.
And so is many semi-colons a "smell". In exactly the same sense, and
with exactly the same justification, yet I don't here anyone saying
"more than one semi-colon indicates poor style" (or any weaker version
of the same observation).
Apparently you haven't hung out with old smalltalk developers or XP
refactoring folks heavely influenced by smalltalk style. "smalltalk
style" usually results in really small routines on the order of
a few lines including loop definitions and conditions which don't have
semicolons. I would consider that a weaker version of a one-semi
observation.
While a style of keeping things to a reasonable handful might result in:
Foo foo( Bar1 b1, Bar2 b2 ) {
if ( b2 == null || b1 == null ) {
throw new IllegalArgumentException( ... );
}
if ( !b2.fooable() || !b1.fooable() ) return null;
for ( ... } {
if ( ... ) {
blah...;
blah...;
if ( ... ) return firstFoo;
}
}
for ( ... ) {
if ( ... ) {
return secondFoo;
}
}
if ( finalTest( ... ) ) {
return goodFoo;
}
return sorryNothingBetterFoo;
}
Hmm, a simple handful yes. No attempt to carry around
extra flags to fall to the bottom, but no smalltalk influenced
developer would stand for not refactoring that to a set of
2 or 3, 2 return methods.
Foo foo( Bar1 b1, Bar2 b2 ) {
if ( b2 == null || b1 == null ) {
throw new IllegalArgumentException( ... );
}
if ( !b2.fooable() || !b1.fooable() ) return null;
Foo result = searchForFirst(...);
if ( result != null ) return result;
result = searchForSecond(...);
if ( result != null ) return result;
result = findFinal( ... );
return result;
}
And just to be slightly annoying, I see little wrong with
a one return version.
Foo foo( Bar1 b1, Bar2 b2 ) {
if ( b2 == null || b1 == null ) {
throw new IllegalArgumentException( ... );
}
Foo result = NothingBetterFoo();
if ( b2.fooable() && b1.fooable() ) {
result = searchForFirst(...);
if ( result == null ) {
result = searchForSecond(...);
if ( result == null ) {
result = findFinal( ... );
}
}
}
return result;
}
Of course this one return case is often not
what often happens (despite the theory that it could),
but is the result of my simplified example.
When I say "little wrong", I mean that
if I came across this code I would not
feel compelled to refactor by straigtening it out.
But if a new case came up, the indent depth is
starting to get smelly.
By the way "poor style" does not equal "smelly".
Smelly is a scale, poor style sounds more like
a boolean. Even when to refactor duplication
can be viewed as sliding scale
Duplication Refactoring Threshold see
http://c2.com/cgi/wiki?DuplicationRefactoringThreshold
I've worked with far too much really
crap code that has been produced with such
a prejudice in mind.
Hmm, my exerpiance may be different. The real
crap I've seen usually doing too much in one
routine (or class). I'm not sure I ever saw anyone
try to keep religiously to one and only one return
such that it was the only reason for bad code,
but I could believe such orthodox attitudes
exist.
cheers,
-Paul
-Paul
