How to tackle a 'branched' arrow head anti-pattern? [duplicate]

Question

This question already has answers here:

Style for control flow with validation checks (3 answers)

Approaches to checking multiple conditions? [duplicate] (3 answers)

Closed 7 years ago.

I recently read this question that features, the arrow anti-pattern.

I have something similar in code I'm trying to refactor except that it branches. It looks a little something like this:

if(fooSuccess==true&&barSuccess!=true){
    if(mooSuccess==true){
           .....
    }else if (mooSuccess!=true){
           .....
    }
}else if(fooSuccess!=true&&barSuccess==true){
    if(mooSuccess==true){
           .....
    }else if (mooSuccess!=true){
        if(cowSuccess==true){
                    .....
        }else if (cowSuccess!=true){
                    .....
        }
    }
}
......

In short it looks like this

if
   if
     if
       if
         do something
       endif
    else if
     if
       if
         do something
        endif
    endif
    else if
     if
       if
         do something
        endif
    endif
 endif

_{Outline borrowed from Coding Horror article on the subject}

And the code goes on through different permutations of true and false for various flags. These flags are set somewhere 'up' in the code elsewhere, either by user input or by the result of some method.

How can I make this kind of code more readable? My intention is that eventually I will have a Bean-type object that contains all the choices the previous programmer tried to capture with this branching anti-pattern. For instance, if in the outline of the code above we really do only have three, I have an enum set inside that bean:

enum ProgramRouteEnum{
    OPTION_ONE,OPTION_TWO,OPTION_THREE;

    boolean choice;
    void setChoice(boolean myChoice){
        choice = myChoice;
    }
    boolean getChoice(){
        return choice;
    }
}

Is this an acceptable cure? Or is there a better one?

Answer 1

The code can be simplified like this:

boolean condA = ( fooSuccess  && !barSuccess &&  mooSuccess )
boolean condB = ( fooSuccess  && !barSuccess && !mooSuccess )
boolean condC = (!fooSuccess  &&  barSuccess &&  mooSuccess )
boolean condD = (!fooSuccess  &&  barSuccess && !mooSuccess &&  cowSuccess)
boolean condE = (!fooSuccess  &&  barSuccess && !mooSuccess && !cowSuccess)

if (condA) {
    ....
    return;
}
if (condB) {
    ....
    return;
}

... and so son 

if (condE) {
    ....
    return;
}

In essence:

• Evaluate complex conditions into a single boolean
• Since conditions are exclusive (as it's typically the case in nested arrow heads) you don't have to nest the ifs, just add a return so that no other block is run.
• This reduces the cyclomatic complexity enormously and makes the code trivial.
• Having what exactly gets run in every case, you can even do a Karnaugh table to identify and remove redundant condition combinations, just as they do in logic circuits design. But you didn't provide that in the example.
• Be sure to extract all this logic into its own method so the return statements don't interfere with blocks that have to be run anymay.
• Descriptive names should be chosen for conA..condF.

Answer 2

user61852 has a pretty good answer solving the general problem of simplifying nested conditionals. However, I was intrigued with your proposed state machine-like solution, and wanted to illustrate how that can sometimes be preferable to a set of binary flags.

It all depends on the sequence of obtaining the flags' values and how many combinations are valid. If you have n flags, you have 2ⁿ states. For 4 flags, that's 16 states, and as you observed, only 3 of them may have any useful meaning. In situations like that, using a state variable instead can simplify things greatly.

Let's say you have a lock that will open if 4 keys are pressed in the right order. If you press any key incorrectly, it immediately resets back to the start of the sequence. A very naïve way to implement a keypress handler using binary flags is:

void key_pressed(char key)
{
   if (!key1correct)
   {
      if (key == pin[0])
      {
         key1correct = true;
      }
   }
   else if (!key2correct)
   {
       if (key == pin[1])
       {
           key2correct = true;
       }
       else
       {
           key1correct = false;
       }
   }
   else if (!key3correct)
   {
       if (key == pin[2])
       {
           key3correct = true;
       }
       else
       {
           key1correct = false;
           key2correct = false;
       }
   }
   else
   {
       if (key == pin[3])
       {
           key4correct = true;
       }
       else
       {
           key1correct = false;
           key2correct = false;
           key3correct = false;
       }
   }

   if (key1correct && key2correct && key3correct && key4correct)
   {
       open_lock();
       key1correct = false;
       key2correct = false;
       key3correct = false;
       key4correct = false;
   }
}

A simplified, flattened version using binary flags (like user61852's answer) looks like this:

void key_pressed(char key)
{
    if (!key1correct && key == pin[0])
    {
        key1correct = true;
        return;
    }

    if (key1correct && !key2correct && key == pin[1])
    {
        key2correct = true;
        return;
    }

    if (key1correct && key2correct && !key3correct && key == pin[2])
    {
        key3correct = true;
        return;
    }

    if (key1correct && key2correct && key3correct && key == pin[3])
    {
        open_lock();
        key1correct = false;
        key2correct = false;
        key3correct = false;
        return;
    }

    key1correct = false;
    key2correct = false;
    key3correct = false;
}

That's a lot easier to read, but still in both of these solutions you have states like key2correct && !key1correct that are completely invalid and unused. Whereas using a state variable num_correct_keys instead of the binary flags looks like this:

void key_pressed(char key)
{
    if (key == pin[num_correct_keys])
        ++num_correct_keys;
    else
        num_correct_keys = 0;

    if (num_correct_keys == 4)
    {
        open_lock();
        num_correct_keys = 0;
    }
}

Granted, this is a contrived example, but people often write code like the first version in less obvious situations, sometimes spread across multiple files. Using a state variable often greatly simplifies code, especially when the flags represent a sequence of events.

Answer 3

I'm not sure your example is a particularly good one for this question, but it's possible to condense it to be much simpler and still preserve the same logic:

if(fooSuccess && !barSuccess)
{
    if(mooSuccess)
    {
        // .....
    }
    else
    {
        // .....
    }
}
else if (!fooSuccess && barSuccess)
{
    if(mooSuccess)
    {
        // .....
    }
    else if(cowSuccess)
    {
        // .....
    }
    else 
    {
        // .....
    }
}

Notice how there's no more than one level deep of ifs. To clean this up, I took the following steps:

• Never check whether a boolean value is == true, != true, or != false. Occasionally I've found it worth checking whether it == false to make it more apparent that that's the expected case, but even that should be exceptional.
• If your else block consists of nothing but an if/else block, then you can merge it with the parent else.
• If your if statement only has one input/variable, you don't need to check the inverse for the else.