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

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; } } 

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.

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"); } } 

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.

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.