9
\$\begingroup\$

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?

\$\endgroup\$
3
  • \$\begingroup\$ is the code for Component available? I think there-in lies your problem. Shouldn't this logic be right in that class? \$\endgroup\$
    – Thufir
    Sep 10, 2013 at 12:25
  • \$\begingroup\$ Can you prove that these are mutually exclusive? Not all statements include hasOneChannel, and hasTwoChannels. \$\endgroup\$
    – aglassman
    Sep 11, 2013 at 16:46
  • \$\begingroup\$ i think is good to reduce duplicate code,but Cat24Condition is not readable.you should think a domain name \$\endgroup\$
    – 王奕然
    Jul 22, 2015 at 1:34

6 Answers 6

5
\$\begingroup\$

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 :)

\$\endgroup\$
3
  • 2
    \$\begingroup\$ +1, if encapsulating complexity is the goal. However if the set of conditions is changing dynamically at runtime then separate objects may be the way to go. \$\endgroup\$
    – radarbob
    Sep 9, 2013 at 20:49
  • 1
    \$\begingroup\$ @radarbob Agreed, in that case my approach is a little "static". However, one could e.g. create a Map<Int, Int> mapping a properties identifier to a category id, which would also be more dynamic. \$\endgroup\$
    – fresskoma
    Sep 9, 2013 at 21:56
  • 1
    \$\begingroup\$ Agree with the progression, but just wanted to point out that bit lists and bit wise storing of booleans are very cryptic. Use sparingly if at all. Future maintainers might hate you. :) \$\endgroup\$ Jun 2, 2014 at 22:08
5
\$\begingroup\$

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;
   }
 }
\$\endgroup\$
6
  • 1
    \$\begingroup\$ I think that using the EnumSets is very good idea (so +1), however I would argue that creating the Category enum is unnecessary. Directly filling a Map with the mapping of your sets to return value instead of creating the Category enum would strike me as a cleaner solution (as your TODO comment alludes to). \$\endgroup\$ Sep 10, 2013 at 0:51
  • \$\begingroup\$ I was not too keen myself on making Category an enum. I did it as such because there might be more information that could be added to each category on top of just the active flag. For example, the int id for each category. \$\endgroup\$
    – toto2
    Sep 10, 2013 at 1:12
  • \$\begingroup\$ can you go further and add logic to the enum itself? \$\endgroup\$
    – Thufir
    Sep 10, 2013 at 10:43
  • \$\begingroup\$ @Thufir I'm not sure what you mean. But it is possible to add logic to enums. I don't enough about this particular code sample to suggest something useful to add. \$\endgroup\$
    – toto2
    Sep 10, 2013 at 12:09
  • \$\begingroup\$ it just seems like there's some insanity with this Component class, and either in an Enum or Component itself, there should be logic so that the object itself returns this desired output..? \$\endgroup\$
    – Thufir
    Sep 10, 2013 at 12:24
2
\$\begingroup\$

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.

\$\endgroup\$
2
\$\begingroup\$

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");
    }
}
\$\endgroup\$
2
  • \$\begingroup\$ You might as well put the binary properties into a map and pair them with the integers to return. Then you could simply use return map.get(properties);˙. \$\endgroup\$ Sep 11, 2013 at 8:16
  • \$\begingroup\$ @DrH Creating a Map would be missing the point of this solution, which is that the compiler does all the setup. private static final Map<Integer,Integer> propsToCat = new HashMap<Integer,Integer>(); static { propsToCat.put(pppp, nnnn); /* etc */ } would be static initialization overhead. And for what? To take information that is already embedded in the bytecode and load it into a HashMap that probably won't perform any better than a super-efficient tableswitch opcode. \$\endgroup\$ Sep 11, 2013 at 20:30
1
\$\begingroup\$

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.

\$\endgroup\$
1
\$\begingroup\$

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.

\$\endgroup\$

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