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<String, Integer> types = new HashMap<String, Integer>();
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<String, Integer> map, String key) {
if (!map.containsKey(key))
return 0;
return map.get(key);
}
private void increment(Map<String, Integer> 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<T> {
private Map<T, Integer> map = new HashMap<T, Integer>();
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<String> types = new CountingSet<String>();
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!
February 2004 March 2004 May 2004 September 2004 October 2004 January 2005 February 2005 September 2005 October 2005 November 2005 December 2005 January 2006 February 2006 March 2006 June 2006 August 2006 January 2007 February 2007 March 2007 April 2007 September 2007 October 2007 November 2007 December 2007 January 2008 February 2008 March 2008 April 2008 May 2008 June 2008 July 2008 August 2008 September 2008 October 2008 November 2008 December 2008 January 2009 February 2009 March 2009 April 2009 June 2009 August 2009