26

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?

marked as duplicate by gnat, user40980, Kilian Foth, GlenH7, EL Yusubov Jul 24 '13 at 17:41

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
    ... Does it seriously say bool == true and bool != true? That might be worse than bool == false – KChaloux Jul 23 '13 at 13:32
  • 1
    Related from C2 Arrow Anti Pattern – user40980 Jul 23 '13 at 13:53
42

The code can be simplified like this:

enter image description here

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.
  • 7
    I really like this idea, although I would highly recommend choosing good names for condA, condB and so on when implementing this in practice. As an example, though, it doesn't matter. – Bobson Jul 23 '13 at 15:27
  • 1
    @Bobson Sure, better names have to be chosen. I edited the answer to add that. – Tulains Córdova Jul 23 '13 at 15:48
  • 1
    @user61852 for what its worth, might want to watch the edit count to make sure that this doesn't fall into community wiki accidently (its a good answer and future rep for it is a good thing). – user40980 Jul 23 '13 at 18:17
  • @MichaelT Do my own edits ( as original author ) count ? – Tulains Córdova Jul 23 '13 at 18:20
  • 1
    All edits after the first 5 minutes count. From What are “Community Wiki” posts? - one of the triggers is "The post has been edited ten (10) times by the original owner." – user40980 Jul 23 '13 at 18:23
4

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 2n 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.

2

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.

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