I’m refactoring several classes of over 2000 lines each. These are classic god-classes that mediate a handful of stubby little data-centric classes around them. I reduced the first, primary domain class by over 1100 lines, to under 1500, by simply pushing responsibilities around. Adding tests after the fact (using WELC techniques, of course) gave me the confidence to refactor. In just a few hours, I’ve been able to completely eliminate about 700 lines so far out of the 1100 that were shifted elsewhere. I’m loving it!

After cleaning up some really ugly code, I ended up with better (not great) code:

    private boolean containsThreeOptionsSameType() {
       Map types = new HashMap();
       for (Option option : options)
          increment(types, option.getType());
       return count(types, Option.ALPHA) >= 3 || 
              count(types, Option.BETA) >= 3 || 
              count(types, Option.GAMMA) >= 3 || 
              count(types, Option.DELTA) >= 3;
    }
    
    private int count(Map map, String key) {
       if (!map.containsKey(key))
          return 0;
       return map.get(key);
    }
    
    private void increment(Map map, String key) {
       if (!map.containsKey(key))
          map.put(key, 1);
       else
          map.put(key, map.get(key) + 1);
    }

I could clean up the complex conditional on the return statement (perhaps calling a method to loop through all types). Also, the count and increment methods contain a bit of duplication.

More importantly, the count and increment methods represent a missed abstraction. This is very common: We tend to leave little, impertinent, SRP-snubbing, OCP-violating methods lying about, cluttering up our classes.

(Aside: I’m often challenged during reviews about exposing things so I can test them. Long-term, entrenched developers are usually very good about blindly following the simple rule about making things as private as possible. But they’re perfectly willing to throw OO concept #1, cohesion, completely out the window.)

I decided to test-drive the soon-to-be-no-longer-missing abstraction, CountingSet. I ended up with an improved implementation of increment by eliminating the redundancy I spotted above:

    import java.util.*;
    
    public class CountingSet {
       private Map map = new HashMap();
    
       public int count(T key) {
          if (!map.containsKey(key))
             return 0;
          return map.get(key);
       }
    
       public void add(T key) {
          map.put(key, count(key) + 1);
       }
    }

Some developers would be appalled: “You built a cruddy little class with just four lines of real code? And for only one client?” And further: “It’s only a subset of a counting set, where are all the other methods?”

Sure thing, buddy. Here’s my client:

    private boolean containsThreeOptionsSameType() {
       CountingSet types = new CountingSet();
       for (Option option : options)
          types.add(option.getType());
       return types.count(Option.ALPHA) >= 3 || 
              types.count(Option.BETA) >= 3 || 
              types.count(Option.GAMMA) >= 3 || 
              types.count(Option.DELTA) >= 3;
    }

My client now only contains intent. The small ugliness with the parameterized map and boxing is now “information hidden.” I also have a simple reusable construct–I know I’ve had a similar need several times before. As for building and/or using a fully-functional CountingSet, there’s something to be said for creating “adapter” classes with the smallest possible, and most meaningful, interface.

Good enough for now. Don’t hesitate to create your own cute little abstractions!

Comments

Mike Bria July 1, 2009 at 10:36am

I dig this Jeff. “Some developers would be appalled” – f’ em. 🙂

Seriously though, I went through a similar flow a couple months ago that I posted about here if you’re interested.

Cheers,
MB

Jeff Langr July 1, 2009 at 8:08pm

Thanks for the link Mike–nice derivation. Also, many thanks for the InfoQ writeup!

Looking at the above code, I can’t help but continue to shake my head at the loss of expressiveness from not being able to use closures in Java yet. (That’s why I’m back on Ruby this month…)

chaussures nike air max June 15, 2011 at 4:01 am

That’s why I’m back on Ruby this month…

Jacob Zimmerman March 25, 2015 at 10:09 am

This goes along with the idea that private “helper” methods are often a code smell that says you need to delegate that responsibility to a different class.

Share your comment