Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

This question already has an answer here:

I was cruising around the programming blogosphere when I happened upon this post about GOTO's:

http://giuliozambon.blogspot.com/2010/12/programmers-tabu.html

Here the writer talks about how "one must come to the conclusion that there are situations where GOTOs make for more readable and more maintainable code" and then goes on to show an example similar to this:

if (Check#1)
{
    CodeBlock#1
    if (Check#2)
    {
        CodeBlock#2
        if (Check#3)
        {
            CodeBlock#3
            if (Check#4)
            {
                CodeBlock#4
                if (Check#5)
                {
                    CodeBlock#5
                    if (Check#6)
                    {
                        CodeBlock#6
                        if (Check#7)
                        {
                            CodeBlock#7
                        }
                        else
                        {
                            rest - of - the - program
                        }
                    }
                }
            }
        }
    }
}

The writer then proposes that using GOTO's would make this code much easier to read and maintain.

I personally can think of at least 3 different ways to flatten it out and make this code more readable without resorting to flow-breaking GOTO's. Here are my two favorites.

1 - Nested Small Functions. Take each if and its code block and turn it into a function. If the boolean check fails, just return. If it passes, then call the next function in the chain. (Boy, that sounds a lot like recursion, could you do it in a single loop with function pointers?)

2 - Sentinal Variable. To me this is the easyest. Just use a blnContinueProcessing variable and check to see if it is still true in your if check. Then if the check fails, set the variable to false.

How many different ways can this type of coding problem be refactored to reduce nesting and increase maintainability?

share|improve this question

marked as duplicate by gnat, Kilian Foth, Jimmy Hoffa, psr, MichaelT Aug 3 '13 at 0:42

This question has been asked before and already has an answer. If those answers do not fully address your question, please ask a new question.

2  
None. Check#1 is not a valid boolean expression in any language I'm aware of. –  Crazy Eddie Feb 14 '11 at 20:39
1  
This seems like it would be better served if it were asked on SO. –  Walter Feb 14 '11 at 20:39
1  
That is always a question in my mind, when I should use SO and when I should post here. Others seem to think I get it wrong a lot. :-) –  saunderl Feb 14 '11 at 20:42
13  
Crazy Eddie - Pseudo Code Dude. Not so literal. –  saunderl Feb 14 '11 at 20:43
2  
pseudo-code that doesn't say anything. Knowing what is being checked would be of some minor importance to how one might refactor it. –  Crazy Eddie Feb 14 '11 at 20:53

12 Answers 12

up vote 13 down vote accepted

It is really hard to tell without knowing how the different checks interact. Rigorous refactoring might be in order. Creating a topology of objects that execute the correct block depending on their type, also a strategy pattern or state pattern might do the trick.

Without knowing what to do best I would consider two possible simple refactorings that could be further refactored by extracting more methods.

The first one I don't realy like since I always like as litle exit points in a method (preferably one)

if (!Check#1)
{ 
    return;
}
CodeBlock#1

if (!Check#2)
{
    return;
}
CodeBlock#2
...

The second one remove's the multiple returns but also adds a lot of noise. (it basicly only removes the nesting)

bool stillValid = Check#1
if (stillValid)
{
  CodeBlock#1
}

stillValid = stillValid && Check#2
if (stillValid)
{
  CodeBlock#2
}

stillValid = stillValid && Check#3
if (stillValid)
{
  CodeBlock#3
}
...

This last one can be refactored nicely into functions and when you give them good names the result might be reasonable';

bool stillValid = DoCheck1AndCodeBlock1()
stillValid = stillValid && DoCheck2AndCodeBlock2()
stillValid = stillValid && DoCheck3AndCodeBlock3()

public bool DoCheck1AndCodeBlock1()
{
   bool returnValid = Check#1
   if (returnValid)
   {
      CodeBlock#1
   }
   return returnValid
}

All in all there are most likely way better options

share|improve this answer
2  
At this moment example1 exits when check#1 passes... should probably read if(!check#1){return;}codeblock#1; –  Inca Jun 19 '11 at 11:07
2  
Readability of the first refactoring is unbeatable. It also help reduce unneeded indentation. I use is as much as I can. –  Ando Jun 19 '11 at 12:05
    
The first refactoring is not quite correct, you can get to check2 without check1 being true, and it seems that both have to be true. –  Marcelo Jun 20 '11 at 2:21
1  
@Inca and @Marcelo Hernández Rishmawy corrected, thanks ! –  KeesDijk Jun 20 '11 at 9:52

This may already have been mentioned, but my "go-to" (pun intended) answer to the "arrowhead antipattern" shown here is to reverse the ifs. Test the opposite of the current conditions (easy enough with a not operator) and return from the method if that is true. No gotos (though pedantically speaking, a void return is little more than a jump to the line of calling code, with the trivial extra step of popping a frame off the stack).

Case in point, here's the original:

if (Check#1)
{
    CodeBlock#1
    if (Check#2)
    {
        CodeBlock#2
        if (Check#3)
        {
            CodeBlock#3
            if (Check#4)
            {
                CodeBlock#4
                if (Check#5)
                {
                    CodeBlock#5
                    if (Check#6)
                    {
                        CodeBlock#6
                        if (Check#7)
                        {
                            CodeBlock#7
                        }
                        else
                        {
                            rest - of - the - program
                        }
                    }
                }
            }
        }
    }
}

Refactored:

if (!Check#1) return;

CodeBlock#1

if (!Check#2) return;

CodeBlock#2

if (!Check#3) return;

CodeBlock#3

if (!Check#4) return;

CodeBlock#4

if (!Check#5) return;

CodeBlock#5

if (!Check#6) return;

CodeBlock#6

if (Check#7)
    CodeBlock#7
else
{
    //rest of the program
}

At each if, we basically check to see if we should continue. It works exactly the same way as the original with only one level of nesting.

If there's something beyond the end of this snippet that should also be run, extract this code into its own method and call it from wherever this code currently lives, before proceeding to the code that would come after this snippet. The snippet itself is long enough, given sufficient actual LOC in each code block, to justify splitting out several more methods, but I digress.

share|improve this answer
    
While I don't subscribe to the "only one exit point" philosophy, I believe functions should when practical be divided into three sections; only the middle section should have side-effects, and it should not have any returns. To my mind, a middle-section return is worse than a goto, since an outdented label the latter will all attention to the fact that there's a goto lurking about. –  supercat Mar 27 at 15:46

Personally, I like wrapping these if statements into separate functions which return a bool if the function succeeds.

The structure looks as follows:


if (DoCheck1AndCodeBlock1() && 
    DoCheck2AndCodeBlock2() && 
    DoCheck3AndCodeBlock3()) 
{
   // ... you may perform the final operation here ....
}

The only drawback is that these functions will have to output data using attributes passed by reference or member variables.

share|improve this answer

From here:

Quite often when I'm looking at a set of pac-man ifs I find that if I just draw out something like a truth table of all the conditions involved I can work out a much better route to resolving the problem.

That way you can also assess whether there is a better method, how you might break it down further and ( and this is a big one with this kind of code ) whether there are any holes in the logic.

Having done that you can probably break it down into a couple of switch statements and a couple of methods and save the next poor mook who has to go through the code a whole lot of problems.

share|improve this answer

If you've routinely got logic that actually requires this pyramid of if checks, you are probably (metaphorically) using a wrench to hammer nails. You'd be better served doing that kind of twisted, complicated logic in a language that supports that kind of twisted and complicated logic with better structures than linear if/else if/else-style constructs.

Languages that might be better-suited to this kind of structure could include SNOBOL4 (with its bizarre dual-GOTO style branching) or logic languages like Prolog and Mercury (with their unification and backtracking capabilities, not to mention their DCGs for rather succinct expression of complicated decisions).

Of course if this is not an option (because most programmers are, tragically, not polyglots) the ideas others have come up with are good like using various OOP structures or breaking up the clauses into functions or even, if you're desperate, and don't mind the fact that most people find them unreadable, using a state machine.

My solution, however, remains reaching for a language that permits you to express what logic you're trying to express (assuming this is commonplace in your code) in an easier, more readable fashion.

share|improve this answer

If you want a possible object orientated solution, the code looks like a canonical example for refactoring using the chain of responsibility design pattern.

share|improve this answer

Using an OO approach a composite pattern where leaf is simple condition and component a union of simple elements make this kind of code extensible and adaptable

share|improve this answer

That is called "Arrow Code" because of the shape of the code with proper indenting.

Jeff Atwood had a good blog post on Coding Horror about how to flatten out the arrows:

Flattening Arrow Code

Read the article for the full treatment, but here are the major points..

  • Replace conditions with guard clauses
  • Decompose conditional blocks into seperate functions
  • Convert negative checks into positive checks
  • Always opportunistically return as soon as possible from the function
share|improve this answer
    
+1: Also convert positive checks into negative checks and use early returns (not all coding standards allow this.) –  oosterwal Feb 14 '11 at 23:03
    
Yeah, we called it 'skyscraper code' - if you turn the code sideways, it looks like high rise buildings... –  Michael Durrant Nov 6 '13 at 12:41

It depends a lot on the size of each code block and the total size, but I tend to split either by a straight "extract method" on a block, or by moving the unconditional parts into separate methods. But the caveats @KeesDijk mentioned apply. The former approach gives you a series of functions each like

if (Check#1)
{
    CodeBlock#1
    NextFunction
}

Which can work well but does lead to code bloat and the "method only used once" antipattern. So I generally prefer this approach:

if (CheckFunctionOne)
{
    MethodOneWithDescriptiveName
    if (CheckFunctionTwo)
    {
        MethodTwoWithDescriptiveName
        ....

With appropriate use of private variables and parameter passing it can be done. Having glanced at literate programming can help here.

share|improve this answer

I know some people will argue that it's a goto, but return; is the obvious refactoring, i.e.

if (!Check#1)
{ 
        return;
}
CodeBlock#1
if (!Check#2)
{
    return;
}
CodeBlock#2
.
.
.
if (Check#7)
{
    CodeBlock#7
}
else
{
    rest - of - the - program
}

If it really is just a bunch of guard checks before running the code, then this works fine. If it's more complicated than that, then this will only make it a bit simpler, and you need one of the other solutions.

share|improve this answer
1  
I don't think this is valid at all... based on the logic shown above, up to all 7 code blocks could execute. In yours, only one code block could ever execute. –  FrustratedWithFormsDesigner Feb 14 '11 at 20:56
    
You only need to tweak your logic and re-organize, return if !Check#1. –  Nim Feb 14 '11 at 21:07
1  
@FrustratedWithFormsDesigner Yes, this doesn't match the original. So it should be more like this... –  Richard Gadsden Feb 14 '11 at 21:11
2  
@moz Uhh, then what is refactoring? Adjusting whitespace? This example works just like the original. –  Jay Feb 14 '11 at 21:40
    
@Jay - I edited it per the suggestion - click on the edited link to see how I got it wrong the first time. –  Richard Gadsden Feb 16 '11 at 10:42

That spaghetti code seems like the perfect candidate for refactoring it into a state machine.

share|improve this answer
1  
Oh, a state machine ... I like that idea. In a way, the code is a Deterministic finite-state machine (DFA) –  saunderl Feb 14 '11 at 20:45
    
+1 I'd go for a state machine too –  Gary Rowe Feb 14 '11 at 20:48

Splitting them out into functions might help?

You call the first function and when it completes it will call the next, the first thing the function would do is test the check for that function.

share|improve this answer
1  
Depends on what else is going on. I've seen code like this "cleaned up" into functions, and the entire purpose becomes lost in a hideous unmaintainable flow-of-control that is distributed into the functions. –  quickly_now Jun 20 '11 at 7:49

Not the answer you're looking for? Browse other questions tagged or ask your own question.