I often encounter code like this:
The excessive nesting of conditional clauses pushes the code out into an arrow formation:
also many times it is possible to combine the “ifs” into a single if:
if (something) {
if (somethingelse) {
if (somemore) {…
into
if (something || somethingelse || somemore) {
…
and some times is even better to create a function to validate those, so it would be
if (DataIsValid(something, somethingelse, somemore)) {…
I have to tell you, number 4 chafed a little bit. I have always avoided multiple returns from a function like the plague.
That being said, I jumped into McConnell (Code Complete 2) right after reading your post today and found what he had to say: “Use a return when it enhances readability: In certain routines, once you know the answer, you want to return it to the calling routine immediately. If the routine is defined in such a way that it doesn’t require any further cleanup once it detects an error, not returning immediately means that you have to write more code.”
McConnel’s example:
Comparison Compare(int value1, int value2)
{
if ( value1 value2 )
return Comparison_GreaterThan;
else
return Comparison_Equal;
}
but he also says this:
"Minimize the number of returns in each routine: It’s harder to understand a routine when, reading it at the bottom, you’re unaware of the possibility that it returned somewhere abouve. For that reason, use returns judiciously–only when they improve readability. "
Now, I have to say, I’m a fairly green developer, but this somehow went against a hard coded rule I had. One function – one return.
Long story short (too late), I learned something today. My little rule was wrong and needed to be rewritten. I hadn’t got that far in the book yet, so hearing it from you AND the gospel (McConnell) will have me trying to change my ways. Thanks!
So THAT’S what the preview button is for…
Should have been this:
McConnell’s example:
Comparison Compare(int value1, int value2)
{
if ( value1 LT value2 )
return Comparison_LessThan;
else if ( value1 GT value2 )
return Comparison_GreaterThan;
else
return Comparison_Equal;
}
I love to see things like this posted on the web. When I was back in school, it was always a major deal if you didn’t follow the strict CS rule of only having one return point for your function.
I use to argue that it didn’t do you any good if it made your code harder to read, but I would always get into trouble anyway.
It’s interesting to see the difference between coding theory and coding practice.
also many times it is possible to combine the “ifs” into a single if:
Yes, although that is a bit of a false economy; the code is “flatter” visually but the if blocks are still present; they were just consolidated into one line. As you pointed out, the best solution is to consolidate the logic into an external function with a proper name, thus reducing the overall per-function cyclomatic complexity.
I have to tell you, number 4 chafed a little bit. I have always avoided multiple returns from a function like the plague.
I’m glad to hear this got you thinking about it!
The two Code Complete guidelines you cite aren’t mutually exclusive…
That said, a perfectly flat function with 53 returns is just as pathological as a 3,000 character wide arrow function with a single return. The difference between the two is where our “mad development skillz” come in.
I think the goal is more readable code (which just happens to be served by a vertical layout more than a horizontal layout). I prefer a function in which I don’t have to scroll at all.
I like your point about using guard clauses. That sort of fits into the Fowler philosophy of failing fast (http://www.martinfowler.com/ieeeSoftware/failFast.pdf).
Get all the failure conditions out of the way at the top of the method so the body can focus on the real meat.
As for 1 return point, I think that’s a bit outdated with code that now has structured exception handling. If you think about it, you have several “return points”.
if(!SomethingNeeded)
throw new CustomException(“message”);
That there is a potential return point.
Though I do agree that keeping it to a minimum is important, there are certain places where a return or a break can be expected that make the overall code more readable.
Kludger, your rule is actually only advice on how to achieve a goal. The goal is “understandable code”.
Using one return value is one way to get understandable code. Having methods short enough to fit on a screen is another. Do whatever you need to do to make the code more understandable, and if that means you do early exit from a function, big deal.
You need to consider the cost of throwing an exception is roughly 10000x the cost of executing an if statement.
Sigh… The good old premature optimization. As well, in some languages, throwing exceptions is actually not expensive at all.
foobar -
Making a good policy on how to use exceptions is not a premature optimization. If you are exposing functionality that can only be reached by catching an exception then (on most platforms) you have a bad design. I’m not claiming devs shouldn’t use exceptions, but they shouldn’t fall into the same Parse vs TryParse trap the .Net runtime fell into before clr 2.0.
Hi,
first the thing that has to be said since - like ages ago: Your blog rocks. In the year I’m now subscribed to your RSS feed I never even once encountered a posting that did not at least remotely interest me. Well written, nice topics. Grats!
Second: I so agree with you. Code with deeply stacked IF’s is terribly hard to read and understand.
Some experience with exactly this problem can be found in a new blog entry of mine inspired by you:
http://www.gnegg.ch/archives/flattening_arrow_code.html
Philip
Are exceptions control structures? My answer is “no.” Maybe I’m just an old fuddy-duddy but throwing exceptions just to guard the execution of code hides the responsibility evaluate and react to conditions but doesn’t make it any easier to satisfy.
As a practical application that combines the guard clause with the “return soonest” philosophy, one ASP.NET pattern I find myself using it a lot is in code like this:
Sub Button1_Click(sender As Object, e As EventArgs)
nbsp; nbsp;nbsp;If Not Page.IsValid Then
nbsp;nbsp;nbsp;nbsp;nbsp;nbsp;Return
nbsp;nbsp;nbsp;End If
nbsp;nbsp;nbsp;’ Actual handler code
End Sub
… or the like.
Hi Jeff,
Correct my if I’m wrong but your suggested refactorings, whilst making the code read better, won’t actually reduce cyclomatic complexity?
When refactoring one must be careful to preserve semantics. Nested if clauses are conjunctions, not disjunctions:
if (something) {
if (somethingelse) {
if (somemore) {…
actually combines into
if (something somethingelse somemore) {
All the clauses must be true for the inner block to execute.
Introducing exceptions also change the contract of the function. There were no exceptions before. Now you need to write catch handlers in all calling code. You need to consider the cost of throwing an exception is roughly 10000x the cost of executing an if statement.
A better refactoring would be to extract the inner if clauses into a separate function, thus breaking off the tip of the arrow:
if (drc[rowIdx].Table.Columns.Contains(“avalId”))
{
do
{
BrokenOffBit( Attributes, attrVal, isChecking, rowIdx );
rowIdx++;
}
while(rowIdx rowCount GetIdAsInt32(drc[rowIdx]) == Id);
}
else
rowIdx++;
If you are exposing functionality that can only be reached by catching an exception then (on most platforms) you have a bad design.
This is a wholly different issue than not using exceptions because they’re “10000x” slower than an if statement. I suppose that means that .NET is “10000x” slower than some other language - look at all the types that throw exceptions! They should have given me an HRESULT instead. What were they thinking?
There was a lot of good information in this post, however I too, like some of the other posters, like to only return from a function in one spot.
With a well written nested if statement, it is extremely easy to deallocate memory and/or do other cleanup at the correct times. With “return” statements sprinkled through a function, it gets hard to ensure you don’t leak memory. Especially if you edit a previously written function, you will have to find all the return statements and figure out whether memory should be freed there.
In languages where memory mangement is done for you (i.e. “It drops out of scope and the system frees it for me”) this is somewhat less of a problem, but then tends to make programmers lazy and forget times where the DO have to clean up after themselves.
Call me archaic, but combining if’s can be bad in many cases.
It often makes the code less readable. If the multiple items in the if are not logically grouped, then following that logic can be fuzzy sometimes.
Not all languages support short-circutting. VB Classic and VBA are prime examples. Combining IF’s can lead to bugs and slower code in those cases.
Regarding the multiple exit points, I agree completely.
I inherited a partially written application from someone. In looking through the code, I found a function that went something like this (Delphi code):
strRet := ‘Fred’;
if (somecondition) then
strRet := ‘Something’;
if strRet = ‘Fred’ then
if (someothercondition) then
strRet := ‘Something else’;
if strRet = ‘Fred’ then
// and so forth for 30 additional conditionals
Result := strRet;
I asked the original developer why he’d done it this way. The response? “There should be only one exit point.”
Needless to say, all of the tests against ‘Fred’ soon disappeared, and there certainly are more exit points.
CodeRush/Refactor make the process of working with these kinds of blocks easier, and refactoring them quickly. Structural Highlighting (CodeRush) draws lines between the start and end of the block, which makes them more clear. With Refactor, you can use the “Replace Nested Conditional with Guard Clause” refactoring to easily flatten your code for better readability, or the “Extract Method” refactoring to move part of the block to its own method. If you’re interested, check it out at http://www.devexpress.com/products/NET/Coderush/. Definitely check out the “CodeRush Training” link from that page, and download the trial to see it for yourself.
Disclaimer: I’m just a happy CodeRush/Refactor user, and have no commercial interest in either.