smell of if

code smell is a hint, a clue which indicates that a potential problem is hidden somewhere in the code. Some of them are well-known by every developer, like too long identifiers, too short identifiers, duplicated code, too many parameters, dead code, etc.

Today, I’m going to tell you about a particular code smell that I recently had to fight against. Failing to find an official term, I will simply call it the if-else tree.

How to recognize an if-else tree?

It is pretty easy to realize that you are facing an if-else tree.

I think the most common situation is when you need to change a very small thing in a well-tested and trustworthy class.

For example say that, somewhere in the project, there is a loop on a list of plugins where each plugin have to provide a result according to a given parameter. The plugins would have to test the parameter and return the wanted result. You did not write the targeted plugin class, but you feel very confident in the fact that this is going to be a quick-and-easy modification. You will probably have to modify a single line.

So you open the file…and find yourself in front of a 1000-line function composed of tens of nested if-else-switch blocks : you have just found an if-else tree!

How do if-else trees appear?

This is unclear how if-else trees have survived to years of programming methodology improvement and still sporadically appear in nowadays code. My theory is that there are two major causes:

  1. The developer thinks that the only way to cope with a situation where there are a lot of possibilities is to find the best if-else tree. The guy usually spends an incredible amount of time trying to optimize the tree, isolating the greatest common factor in each tree level and writing the most complex condition possible, supposedly to speed up the execution. In this case the resulting code is a completely arcane entity with a nearly unrefactorable workflow
  2. The code was built up step-by-step without being refactored. The developer thinks: “Ok, they want me to call this function when the array is empty or exceeds the maximum number of items: let’s surround the code by an if-else” or “Well, there used to be two types for this parameter, now they added seven other types: let’s add the corresponding if-else or switch cases.” In this case the code keeps growing every time we try to change it, like a gluttonous monster.

A bad smell?

Why having an if-else tree in a function is a bad code smell? There are several reasons for that :

  1. The function is long (sic)
  2. The logic is difficult to understand
  3. Coupling is very important
  4. Too much responsibilities for a single function, the logic is fully contained in a one function

These four reasons are enough for a code smell : the probability of a hidden bug somewhere is important, the code is nearly unmaintainable and all the decisions are made in a single function.

Uproot an if-else tree

Although there are several ways to completely destroy some of the if-else constructs, I am going to focus on pure object-oriented solutions. To uproot if-else trees we will use well-known design patterns. Note that the examples presented bellow are here for pedagogical purpose only, and thus will not reflect the complexity of an actual if-else tree. Our goal is to demonstrate tools, not to show real life examples.

Decorator

Here is a small example of an if-else tree that can be removed using the Decorator pattern.

Say you are facing this javascript object:

function Burger (with_cheese, with_salad, with_ketchup) {
  this.price = function() {
    var p = 2.5;
    if (with_cheese) {
      p += 0.5;
    }
    if (with_salad) {
      p += 0.2;
    }
    if (with_ketchup) {
      p += 0.2;
    }
  }
}

One can easily rewrite it using the Decorator pattern:

function Burger () {
  this.price = function() {
    return 2.5;
  }
}

function BurgerWithCheese(burger) {
  this.price = function() {
    return burger.price() + 0.5;
  }
}

function BurgerWithSalad(burger) {
  this.price = function() {
    return burger.price() + 0.2;
  }
}

function BurgerWithKetchup(burger) {
  this.price = function() {
    return burger.price() + 0.2;
  }
}

Now

new Burger (true,true,false);

becomes

new BurgerWithCheese( new BurgerWithSalad( new Burger() ) );

and the tree has gone away. This technique is particularly useful when we need to add new features to an existing class : we avoid breaking anything and add the features “around” the existing class.

Strategy

Still in javascript, we have this if-else tree

var sauce_ketchup = 1;
var sauce_mayonnaise = 2;
var sauce_pepper = 3;
var sauce_none = 0;

function Burger (sauce, with_salad) {
 this.price = function() {
 var p = 2.5;
 if (sauce == sauce_ketchup || sauce == sauce_mayonnaise) {
 p += 0.2;
 } else if (sauce == sauce_pepper ) {
 p += 0.3;
 }
 if (with_salad) {
 p += 0.2;
 }
 return p;
 }
}

We use the Strategy pattern to get rid of the sauce if-else tree:

function Burger (sauce_strategy, salad_strategy) {
 this.price = function() {
 return 2.5 + sauce_strategy.price() + salad_strategy.price();
 }
}

function KetchupSauceStrategy () {
 this.price = function() {
 return 0.2;
 }
}
function MayonnaiseSauceStrategy () {
 this.price = function() {
 return 0.2;
 }
}
function PepperSauceStrategy () {
 this.price = function() {
 return 0.3;
 }
}
function NoneSauceStrategy () {
 this.price = function() {
 return 0;
 }
}

function WithSaladStrategy () {
 this.price = function() {
 return 0.2;
 }
}
function NoSaladStrategy () {
 this.price = function() {
 return 0;
 }
}

Now

new Burger (sauce_mayonnaise,true);

becomes
new Burger( new MayonnaiseSauceStrategy(), new WithSaladStrategy ());
and the tree is gone again. Although this example is rather simplistic, it demonstrates how to avoid the logic concentration in a single function.

State

Once again a very simple example here : we cook a burger in Java:

class BurgerCook {
 private static int STEP_NOTHING = 0;
 private static int STEP_BREAD = 1;
 private static int STEP_MEAT = 2;
 private static int STEP_SALAD = 3;
 private static int STEP_SAUCE = 4;
 private static int STEP_DONE = 5;

 private int step = STEP_NOTHING;
 private boolean has_already_put_bottom_bread = false;

 private void next () {
 switch (step) {
   case STEP_NOTHING:
     System.out.println("Let's start cooking!");
     step = STEP_BREAD;
     break;
   case STEP_BREAD :
     System.out.println("Putting the bread");
     if (has_already_put_bottom_bread ) {
       step = STEP_DONE;
     } else {
       has_already_put_bottom_bread = true;
       step = STEP_SALAD ;
     }
     break;
   case STEP_MEAT :
     System.out.println("Putting the meat");
     step = STEP_SAUCE;
     break;
   case STEP_SALAD :
     System.out.println("Putting the salad");
     step = STEP_MEAT;
     break;
   case STEP_SAUCE :
     System.out.println("Putting the sauce");
     step = STEP_BREAD;
     break;
   case STEP_DONE :
     System.out.println("Burger done");
     break;
   default:
     System.out.println("Burger ain't chili");
     break;
   }
 }

 public void cook() {
   step = STEP_NOTHING;
   has_already_put_bottom_bread = false;
   while (step != STEP_DONE) {
     next();
   }
 }
}

With the State pattern, we will get rid of the switch and distribute the responsibilities:

interface BurgerCookingState {
 public void next ( BurgerCook cook );
}

class Nothing implements BurgerCookingState {
  public void next ( BurgerCook cook ) {
    System.out.println("Let's start cooking!");
    cook.setState (new BottomBread());
  }
}
class BottomBread implements BurgerCookingState {
  public void next ( BurgerCook cook ) {
    System.out.println("Adding the bottom bread");
    cook.setState (new Salad());
  }
}
class Salad implements BurgerCookingState {
  public void next ( BurgerCook cook ) {
    System.out.println("Adding the bread");
    cook.setState (new Meat());
  }
}
class Meat implements BurgerCookingState {
  public void next ( BurgerCook cook ) {
    System.out.println("Adding the meat");
    cook.setState (new Sauce());
  }
}
class Sauce implements BurgerCookingState {
  public void next ( BurgerCook cook ) {
    System.out.println("Adding the sauce");
    cook.setState (new TopBread());
  }
}
class TopBread implements BurgerCookingState {
  public void next ( BurgerCook cook ) {
    System.out.println("Adding the top bread");
    cook.setState (new Done());
  }
}
class Done implements BurgerCookingState {
  public void next ( BurgerCook cook ) {
    System.out.println("Burger done");
    cook.setFinished(true);
  }
}

class BurgerCook {
  boolean finished = false;
  BurgerCookingState state;

  public void cook() {
    state = new Nothing(this);
    finished = false;
    while (!finished ) {
      state.next();
    }
  }
}

Chain of Responsibility

Will we have burgers for diner tonight, in C++?

class Person
{
  ...
  public:
    bool isOnADiet () { ... }
    time_t lastBurger () { ... }
  ...
}

class Mother : public Person
{
  ...
  public:
    bool mustHaveDinerWithMommy () { ... }
  ...
}

class Junior : public Person
{
  ...
  public:
    bool needNotToFeelBloatedTomorrow() { ... }
  ...
}

bool burgersTonight (Junior &junior, Mother &mum,
           Person &dad, bool junior_will_be_there,
           bool dad_is_working_late)
{
  if (junior_will_be_there) {
    if ( junior.isOnADiet()
         || ( time(0) - junior.lastBurger() < 3600 )
         || junior.needNotToFeelBloatedTomorrow() ) {
      return false;
    }
  }

  if (!dad_is_working_late) {
     if ( dad.isOnADiet()
         || ( time(0) - dad.lastBurger() < 86400 ) ) {
       return false;
     }
  }

  if ( mum.isOnADiet()
         || ( time(0) - mum.lastBurger() < 604800)
         || mum.mustHaveDinerWithMommy () ) {
      return false;
    }
  }

  return true;
}

We will use the Chain of Responsibility pattern to remove the logic from burgersTonight and make the if-tree go away:

class Person
{
  protected:
    time_t burger_tolerance;
    ...
  public:
    ...
    virtual bool burgersTonight ()
    {
      return !is_on_diet && (time(0) - last_burger) >= burger_tolerance;
    }
}

class Mother : public Person
{
  ...
  public:
    ...
    virtual bool burgersTonight ()
    {
      return Person::burgersTonight() && !must_have_diner_with_mommy;
    }
}

class Junior: public Person
{
  ...
  public:
    ...
    virtual bool burgersTonight ()
    {
      return Person::burgersTonight() && !need_not_to_feel_bloated_tomorrow;
    }
}

bool burgersTonight (Junior &junior, Mother &mum, Person &dad, bool junior_will_be_there, bool dad_is_working_late)
{
  return
    ( !junior_will_be_there || junior.burgersTonight () ) &&
    ( dad_is_working_late || dad.burgersTonight () ) &&
    mum.burgersTonight ();
}

Conclusion

If you managed to get to this point, I only have to thank you for reading this article entirely, and pray for if-else trees‘s extinction. If you have any questions or remarks, do not hesitate to leave a comment bellow.

Further reading

One thought on “smell of if

Leave a Reply

This site uses Akismet to reduce spam. Learn how your comment data is processed.