33

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?

Community
  • 1
AncientSwordRage
  • 1,033
  • 1
  • 11
  • 24
  • 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](http://www.c2.com/cgi/wiki?ArrowAntiPattern) –  Jul 23 '13 at 13:53

3 Answers3

54

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.
candied_orange
  • 102,279
  • 24
  • 197
  • 315
Tulains Córdova
  • 39,201
  • 12
  • 97
  • 154
  • 8
    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
  • 2
    @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). –  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?](http://meta.stackexchange.com/questions/11740/what-are-community-wiki-posts) - one of the triggers is "The post has been edited ten (10) times by the original owner." –  Jul 23 '13 at 18:23
  • @MichaelT Thanx. That sucks. This answer of mine is "community wiki" even when edits are like "added 3 words to body": http://programmers.stackexchange.com/questions/171024/never-do-in-code-what-you-can-get-the-sql-server-to-do-well-for-you-is-this/171042#171042 – Tulains Córdova Jul 23 '13 at 18:23
  • @user61852 Its in part to avoid people bouncing their question (or answer) to get more rep (it shows up on the top of the active list) by making lots of small edits. I can see the value of this thought. –  Jul 23 '13 at 18:26
  • This is a great answer, but I'll add -- when you name those conditions, you'll likely discover that what you really have are discrete states. And once you have identified states, and think they're relatively stable, you can get major supportability benefits by adopting a State pattern and introducing some form of state machine. – Matthew Mark Miller Jan 03 '16 at 21:33
  • What if you have 11 conditions...or more? I only ask because I actually have 11 conditions and I did that very thing except I didn't declare the conditions at the beginning into a variable. Rather, the if statements contained the conditions and return if not met. It was our solution to the arrow head anti pattern. Interested in other solutions too. – Andy Nov 17 '16 at 09:33
5

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.

Community
  • 1
Karl Bielefeldt
  • 146,727
  • 38
  • 279
  • 479
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.
Bobson
  • 4,638
  • 26
  • 24