Overused Extract Method refactoring

Monday, September 21, 2009 at 22:57
Henrik Gustafsson in Hacking

I was reading the ObjectMentor blogs the other day, and came across a post called "One Thing: Extract till you Drop" by Robert C. Martin. I definitely see the point of using extract method to a great extent in order to clarify the extent of the program, but I felt that example went a bit too far for a number of reasons.

1. The names of the extracted methods made very little sense. Even when knowing what the class does it is hard to figure out the difference between replaceAllSymbols() and replaceAllInstances() and their relations to the other methods. You actually need to read them all in order to puzzle together the call-graph.
2. It introduces mutability in the class quite unnecessarily making reuse of the instance impossible.
3. Some of the extracted methods have strangely asymmetric abstraction levels.

There are also some other issues regarding efficiency and thread-safeness that I reacted strongly to, especially since they are so easy to avoid.

When implementing it myself I thought about the scenarios when this would be used, and I could find two main ones. The first is to use this to iterate over different strings using the same dictionary and the second is to iterate over different dictionaries using the same string. Not knowing anything about the use-case I went for the former.

I made an attempt to improve it in the comments, but I was tired and messed it up a bit. Normally I probably would have solved the same problem in a manner closer to this:

public class SymbolReplacer {
    private static final Pattern pattern =
        Pattern.compile("\\$([a-zA-Z]\\w*)");

    private final SymbolLookup lookup;

    SymbolReplacer(SymbolLookup lookup) {
        this.lookup = lookup;
    }

    public String replace(CharSequence stringToReplace) {
        final Matcher matcher = pattern.matcher(stringToReplace);
        return buildReplacement(matcher).toString();
    }

    private StringBuffer buildReplacement(Matcher matcher) {
        final int bufferLength =
            matcher.regionEnd() - matcher.regionStart();
        final StringBuffer buffer =
            new StringBuffer(bufferLength);

        fillBuffer(matcher, buffer);
        return buffer;
    }

    private void fillBuffer(Matcher matcher, StringBuffer buffer) {
        while (matcher.find())
            matcher.appendReplacement(buffer,
                getMatchReplacement(matcher));
        matcher.appendTail(buffer);
    }

    public String getMatchReplacement(Matcher matcher) {
        final String expansion =
            lookup.lookupValue(matcher.group(1));
        return (expansion != null) ?
            expansion :
            Matcher.quoteReplacement(matcher.group(0));
    }
}

I introduced the interface SymbolLookup as a replacement for the secret getSymbol() method. There are a few things I would like to fix in this as well; the getMatchReplacement() method is not as clear as I would have liked, and the line of abstraction between the different methods/layers is not well defined. Still, provided that SymbolLookup is thread-safe, so is this class. Also, you can reuse the instance to iterate over many strings.

Article originally appeared on Fnordology (http://blog.fnord.se/).