Many if statements - refactoring [closed]

Question

Yesterday I found some nice video from some IT conference about removing if statements from code. I saw a very nice example of refactoring. I am curious what do you think about it, especially what weak points do you see in such approach.

1) We have a class with very "nice" method:

public int inferCategoryID(Component c) {
        if(c.hasOneChannel() && c.hasValidCertificate() && c.isTested() && c.frequentlyUsed()) {
            return 24;
        }
        if(c.hasOneChannel() && c.hasValidCertificate() && !c.isTested() && c.frequentlyUsed()) {
            return 95;
        }
        if(c.hasOneChannel() && !c.hasValidCertificate() && c.isTested() && c.frequentlyUsed()) {
            return 221;
        }
        if(c.hasTwoChannels() && c.hasValidCertificate() && c.isTested() && c.frequentlyUsed()) {
            return 2004;
        }
        if(c.hasTwoChannels() && c.hasValidCertificate() && !c.isTested() && c.frequentlyUsed()) {
            return 20044;
        }
        if(c.hasTwoChannels() && c.hasValidCertificate() && !c.isTested() && !c.frequentlyUsed()) {
            return 2003;
        }
        // etc...
    }

As you can see all conditions are mutually exclusive.

2) The idea was, to create a separate class for each condition - under a common interface:

interface CategoryIDCondition  {
        public boolean isTrue(Component c);
        public int getID();
    }

class Cat24Condition implements CategoryIDCondition {
    public boolean isTrue(Component c){
        return c.hasOneChannel() 
                && c.hasValidCertificate() 
                && c.isTested() 
                && c.frequentlyUsed();
    }

    public int getID(){
        return 24;
    }
}

// etc...

3) Usage example:

public int inferCategoryID(Component c) {
        List<CategoryIDCondition> conditions = new ArrayList<>(); 
        conditions.add(new Cat24Condition);
        conditions.add(new Cat95Condition);
        // etc... (btw. not a good place for list init but never mind...)

        for (CategoryIDCondition cond : conditions) {
            if(cond.isTrue(c)){
                return cond.getID();
            }
        }
    }

What weak points do you see in such approach apart from that, that conditions have to be mutually exclusive, or have to be added to conditions list in a very specific order? Would you decide to use such strategy in your code?

Answer 1

Personally, I wouldn't use that approach, because of the amount of boilerplate code and complexity it introduces. Adding N+1 classes, where N is the number of conditions, just seems like overkill to me.

First off, I don't think that the method at hand must necessarily be refactored. I agree that is not a terribly "nice" method from a aesthetic point of view, but the amount of complexity it contains might not justify a refactor. Often, a bit of reformatting is already a good approach:

public int inferCategoryID(Component c) {
    if( c.hasOneChannel() &&
        c.hasValidCertificate() &&
        c.isTested() &&
        c.frequentlyUsed()) {
        return 24;
    }

    [...]
}

If that is not satisfactory, my next approach would've been to simply used separate methods for the conditions. This doesn't imply creating lots of classes, but still clean up the inferCategoryID substantially. While I'm not a fan of giving names like isCat24, I will use it here since the example refactoring technique using classes does so too and I can't infer more useful naming from the conditions:

public int inferCategoryID(Component c) {
    if(isCat24(c)) {
        return 24;
    }

    [...]
}

public boolean isCat24(Component c) {
    return c.hasOneChannel() &&
           c.hasValidCertificate() &&
           c.isTested() &&
           c.frequentlyUsed();
}

Last but not least, in cases where lots of boolean conditions are used, bit-lists are often applicable. The following code isn't tested, but it should give an idea of what I mean:

const int CATEGORY_24 = 15;
const int CATEGORY_95 = 13;

public int inferCategoryID(Component c) {
    int properties = componentProperties(c);

    if(properties == CATEGORY_24) {
        return 24;
    }

    if(properties == CATEGORY_95) {
        return 95;
    }

    [...]
}

public int componentProperties(Component c) {
    return ((int) c.hasOneChannel() << 3) &&
           ((int) c.hasValidCertificate() << 2) &&
           ((int) c.isTested() << 1) &&
           ((int) c.frequentlyUsed());
}

The integer returned from componentProperties is a dense representation of the components properties in form of a bit list, that is each bit represents one boolean property. For example, if all properties are true, the resulting integer (in binary) would be 1111, if only hasOneChannel was true, the integer would be 1000 and so on. This has the advantage that you don't have to create a large amount of methods or classes, but still cleans up the inferCategoryID method quite nicely. The downside of this method is that it is arguably the most cryptic (while I also find the multiple-class approach relatively cryptic :D).

Since I said that adding lots of classes introduces too much complexity to be really worth it (in this case), I can't say that the last solution is any different in terms of adding unnecessary complexity, so I probably wouldn't use either of them for the use case at hand. I just wanted to give you an idea of a different approach that, in my opinion, is preferable to creating a class per condition :)

Answer 2

There are 4 boolean state-returning methods, but my solution is probably more relevant when there are more states. Instead of having 4 boolean state members, the Component class would have an EnumSet<State>.

public enum State {
   HAS_ONE_CHANNEL, HAS_VALID_CERTIFICATE, IS_TESTED, IS_FREQUENTLY_USED;
}

public enum Category {
   C24(EnumSet.of(States.HAS_ONE_CHANNEL, ...)),
   C95(EnumSet.of(...)),
   ...;

   private EnumSet<State> enumSet;

   private Category(EnumSet<State> enumSet) {
      this.enumSet = enumSet;
   }

   public boolean equals(EnumSet<State> otherEnumSet) {
      return enumSet.equals(otherEnumSet);
   }

   // returns null if not found
   public static Category find(EnumSet<State> otherEnumSet) {
       // TODO instead of looping, it would be more efficient to build a HashMap<EnumSet<State>, Category>> once
       for (Category category : Category.values()) {
          if (category.equals(otherEnumSet)) 
             return category;
       }
       return null;
   }
 }

Answer 3

You basically have two values, that have a one to one coorespondences. I would take advantage of that.

One value is what you want to return, the other value is the distinct value you get by using your 4 (5?) booleans as a bit pattern.

Here's an example of creating the mapping programatically. It is for illustration purposes only, I don't recommend using it -- better to either put the map into a config file or a database.

Answer 4

Here's my solution.

Advantages

• The .hasOneChannel() methods are called just once.
• No runtime setup, and no data structures. Everything is compiled in.
• The switch statement is fast.

Disadvantages

• You have to manually maintain the mapping of bitmap values to constant names. (Well, you could dispense with the named constants and just hard-code the bitmap values as switch cases, but that would just be too cryptic, I think.)
• The definition of the category IDs happens far from the definition of the constant names. It would be nicer to be able to say final Properties FOVT(15, 24).
• Using enums would be more elegant (but more complicated).

‍

/**
 * F = Frequently used          f = Rarely used
 * O = One channel              o = Two channels
 * V = Valid certificate        v = Invalid certificate
 * T = Tested                   t = Untested
 */
private static final int
    FOVT = 15, FOVt = 14, FOvT = 13, FOvt = 12,
    FoVT = 11, FoVt = 10, FovT =  9, Fovt =  8,
    fOVT =  7, fOVt =  6, fOvT =  5, fOvt =  4,
    foVT =  3, foVt =  2, fovT =  1, fovt =  0;

public static int inferCategoryID(Component c) {
    int properties = (c.frequentlyUsed()      ? 1 << 3 : 0) |
                     (c.hasOneChannel()       ? 1 << 2 : 0) |
                     (c.hasValidCertificate() ? 1 << 1 : 0) |
                     (c.isTested()            ? 1 << 0 : 0);
    switch (properties) {
      case FOVT: return 24;
      case FOVt: return 95;
      case FOvT: return 221;
      case FOvt: return ...;
      case FoVT: return 2004;
      case FoVt: return 20044;
      // etc...
      default:
        assert false;  // All 16 cases should be covered
        throw new UnsupportedOperationException("Unrecognized Component");
    }
}

Edit: On second thought, dispensing with the named constants isn't so bad, especially if you use comments and binary literals (Java 7).

public static int inferCategoryID(Component c) {
    int properties = (c.frequentlyUsed()      ? 1 << 3 : 0) |
                     (c.hasOneChannel()       ? 1 << 2 : 0) |
                     (c.hasValidCertificate() ? 1 << 1 : 0) |
                     (c.isTested()            ? 1 << 0 : 0);
    switch (properties) {
      /**
       * F = Frequently used        f = Rarely used
       * O = One channel            o = Two channels
       * V = Valid certificate      v = Invalid certificate
       * T = Tested                 t = Untested
       */

      case 0b1111 /* FOVT */: return 24;
      case 0b1110 /* FOVt */: return 95;
      case 0b1101 /* FOvT */: return 221;
      case 0b1100 /* FOvt */: return ...;
      case 0b1011 /* FoVT */: return 2004;
      case 0b1010 /* FoVt */: return 20044;
      // etc...
      default:
        assert false;  // All 16 cases should be covered
        throw new UnsupportedOperationException("Unrecognized Component");
    }
}

Answer 5

This doesn't seem like an improvement - there is still an if per case, and an extra jump for the for loop. It might be better to factor out parts of the ifs that are in common, giving something more like:

if(condA()) {
  if(condB()) {
    //foo
  } else { //!condB
    //bar
} else { //!condA
  //etc
}

It doesn't remove any ifs, but the number of them that will be tested is now O(log n) instead of O(n), which is an improvement! The major downside is that this is a pain to read and mentally track the flow of, not to mention being more idiomatic of C-style programming.

Answer 6

I would do it just the way the video presented it. It gives very loose coupling to the classes and it provides extensibility without needing to later open a class and modify it. The 'single responsibility principle' is a great guideline for building robust and easy to maintain code and this methodology lends itself to the SRP well.