One thing: Abstract till you drop!

A code smell is some characteristic of your code that indicates that something may not be quite right with its design. We can use them to gently guide us towards better, easier to understand, more maintainable code. One smell I have found particularly useful to pay attention to is “too many private methods” (aka Methods should be public). Having many private methods can often be an indication that you are missing one or more abstractions.

To illustrate this I will take an example reproduced here from Uncle Bob’s post One thing:Extract till you drop I think that Bob’s approach here is excellent, as far as it goes, but that to really get to “One Thing” we need to push on by seeing what our extracted private methods can tell us about possible abstractions, and to ensure that not only do methods do one thing but that Classes as well should do one thing our mantra should be;

Extract till we drop

then

Abstract till we drop

Uncle Bob’s starting point is the following code;

SymbolReplacer.java
abstract class SymbolReplacer { 
    protected String stringToReplace; 
    protected List alreadyReplaced = new ArrayList(); 
 
    SymbolReplacer(String s) { 
        this.stringToReplace = s; 
    } 
 
    String replace() { 
        Pattern symbolPattern = Pattern.compile("\\$([a-zA-Z]\\w*)"); 
        Matcher symbolMatcher = symbolPattern.matcher(stringToReplace); 
        while (symbolMatcher.find()) { 
            String symbolName = symbolMatcher.group(1); 
            if (getSymbol(symbolName) != null && !alreadyReplaced.contains(symbolName)) { 
                alreadyReplaced.add(symbolName); 
                stringToReplace = stringToReplace.replace("$" + symbolName, getSymbol(symbolName)); 
            } 
        } 
        return stringToReplace; 
    } 
 
    protected abstract getSymbol(String symbolName) ; 
}

By following the mantra of “extract till you drop” Bob ends up here;


SymbolReplacer.java
import java.util.ArrayList; 
import java.util.List; 
import java.util.regex.Matcher; 
import java.util.regex.Pattern; 
 
abstract class SymbolReplacer { 
    protected String stringToReplace; 
    protected List alreadyReplaced = new ArrayList(); 
    private Matcher symbolMatcher; 
    private final Pattern symbolPattern = Pattern.compile("\\$([a-zA-Z]\\w*)"); 
 
    SymbolReplacer(String s) { 
        this.stringToReplace = s; 
        symbolMatcher = symbolPattern.matcher(s); 
    } 
 
    String replace() { 
        replaceAllSymbols(); 
        return stringToReplace; 
    } 
 
    private void replaceAllSymbols() { 
        for (String symbolName = nextSymbol(); symbolName != null; symbolName = nextSymbol()) 
            replaceAllInstances(symbolName); 
    } 
 
    private String nextSymbol() { 
        return symbolMatcher.find() ? symbolMatcher.group(1) : null; 
    } 
 
    private void replaceAllInstances(String symbolName) { 
        if (shouldReplaceSymbol(symbolName)) 
            replaceSymbol(symbolName); 
    } 
 
    private boolean shouldReplaceSymbol(String symbolName) { 
        return getSymbol(symbolName) != null && !alreadyReplaced.contains(symbolName); 
    } 
 
    private void replaceSymbol(String symbolName) { 
        alreadyReplaced.add(symbolName); 
        stringToReplace = stringToReplace.replace( 
                symbolExpression(symbolName), 
                getSymbol(symbolName)); 
    } 
 
    private String symbolExpression(String symbolName) { 
        return "$" + symbolName; 
    } 
 
    protected abstract String getSymbol(String symbolName); 
}

Now at this point Uncle Bob reckons things are a lot clearer, we have done some decomposition and methods are only doing one thing. Steven Jeuris disagrees in his post Function Hell, claiming “Due to the amount of functions, the overview of this trivial procedural task is lost”. But I think the truth is Bob’s code does make things a lot clearer; the code smell is clearer, indeed it has ripened to a degree that it reeks this class is doind more than one thing time to invoke the second part of the mantra:

Abstract till you drop!

Not only should each method do only one thing so should each class, as we will see this class is doing at least four things. By decomposing to private methods we have shone a light on this and can now start to pull out classes that do only one thing.

I will now go through step by step how I went about refactoring this code.

The first thing I did was actually to change the re-use pattern from inheritance with the abstract base class to composition by injecting a SymbolTranslator, this is a cleaner form of re-use and payed dividends later.

SymbolReplacer.java
import java.util.ArrayList; 
import java.util.List; 
import java.util.regex.Matcher; 
import java.util.regex.Pattern; 
 
class SymbolReplacer { 
    protected String stringToReplace; 
    private final SymbolTranslator symbolTranslator; 
    protected List alreadyReplaced = new ArrayList(); 
    private Matcher symbolMatcher; 
    private final Pattern symbolPattern = Pattern.compile("\\$([a-zA-Z]\\w*)"); 
 
    SymbolReplacer(String s, SymbolTranslator symbolTranslator) { 
        this.stringToReplace = s; 
        this.symbolTranslator = symbolTranslator; 
        symbolMatcher = symbolPattern.matcher(s); 
    } 
 
    String replace() { 
        replaceAllSymbols(); 
        return stringToReplace; 
    } 
 
    private String nextSymbol() { 
        return symbolMatcher.find() ? symbolMatcher.group(1) : null; 
    } 
 
    private void replaceSymbol(String symbolName) { 
        alreadyReplaced.add(symbolName); 
        stringToReplace = stringToReplace.replace( 
                symbolExpression(symbolName), 
                symbolTranslator.getSymbol(symbolName)); 
    } 
 
    private String symbolExpression(String symbolName) { 
        return "$" + symbolName; 
    } 
 
    private void replaceAllSymbols() { 
        for (String symbolName = nextSymbol(); symbolName != null; symbolName = nextSymbol()) 
            replaceAllInstances(symbolName); 
    } 
 
    private void replaceAllInstances(String symbolName) { 
        if (shouldReplaceSymbol(symbolName)) 
            replaceSymbol(symbolName); 
    } 
 
    private boolean shouldReplaceSymbol(String symbolName) { 
        return symbolTranslator.getSymbol(symbolName) != null && !alreadyReplaced.contains(symbolName); 
    } 
 
}

Highlighted in red above I could see that the symbolMatcher and symbolPattern  attributes were used only by the nextSymbol method, which itself used no other attributes of the object.  Its often the case that where you have more than two attributes some will be more closely related or have more cohesion than others. By decomposing into private methods we have made this much easier to see. So I pulled out an iterator class;

SymbolIterator.java
import java.util.regex.Matcher; 
import java.util.regex.Pattern; 
 
/** 
 * Created by dave on 14/05/15. 
 */ 
public class SymbolIterator { 
    private Matcher symbolMatcher; 
    private final Pattern symbolPattern = Pattern.compile("\\" + SymbolTranslator.SYMBOL_PREFIX + "([a-zA-Z]\\w*)"); 
 
    public SymbolIterator(String s) { 
        symbolMatcher = symbolPattern.matcher(s); 
 
    } 
 
    public String nextSymbol() { 
        return symbolMatcher.find() ? symbolMatcher.group(1) : null; 
    } 
} 

This felt like we were making progress so lets look at the next opportunity;

SymbolReplacer.java
import java.util.ArrayList; 
import java.util.List; 
import java.util.regex.Matcher; 
import java.util.regex.Pattern; 
 
class SymbolReplacer { 
    private final SymbolIterator symbolIterator; 
    protected String stringToReplace; 
    private final SymbolTranslator symbolTranslator; 
    protected List alreadyReplaced = new ArrayList(); 
 
    SymbolReplacer(String s, SymbolTranslator symbolTranslator) { 
        this.stringToReplace = s; 
        this.symbolTranslator = symbolTranslator; 
        this.symbolIterator = new SymbolIterator(s); 
    } 
 
    String replace() { 
        replaceAllSymbols(); 
        return stringToReplace; 
    } 
 
 
    private void replaceSymbol(String symbolName) { 
        alreadyReplaced.add(symbolName); 
        stringToReplace = stringToReplace.replace( 
                symbolExpression(symbolName), 
                symbolTranslator.getSymbol(symbolName)); 
    } 
 
    private String symbolExpression(String symbolName) { 
       return "$" + symbolName; 
    } 
 
    private void replaceAllSymbols() { 
        for (String symbolName = symbolIterator.nextSymbol(); symbolName != null; symbolName = symbolIterator.nextSymbol()) 
            replaceAllInstances(symbolName); 
    } 
 
    private void replaceAllInstances(String symbolName) { 
        if (shouldReplaceSymbol(symbolName)) 
            replaceSymbol(symbolName); 
    } 
 
    private boolean shouldReplaceSymbol(String symbolName) { 
        return symbolTranslator.getSymbol(symbolName) != null && !alreadyReplaced.contains(symbolName); 
    } 
 
}

Remember we pulled out that SymbolTranslator earlier,  it looked to me like symbolExpression had a close relationship with SymbolTranslator so I decided to move it to the SymbolTranslator class. I wasn’t quite sure where this would lead at this point so I just went with it to see where I ended up. SymbolReplacer now looked like this;

SymbolReplacer.java
import java.util.ArrayList; 
import java.util.List; 
import java.util.regex.Matcher; 
import java.util.regex.Pattern; 
 
class SymbolReplacer { 
    private final SymbolIterator symbolIterator; 
    protected String stringToReplace; 
    private final SymbolTranslator symbolTranslator; 
    protected List alreadyReplaced = new ArrayList(); 
 
    SymbolReplacer(String s, SymbolTranslator symbolTranslator) { 
        this.stringToReplace = s; 
        this.symbolTranslator = symbolTranslator; 
        this.symbolIterator = new SymbolIterator(s); 
    } 
 
    String replace() { 
        replaceAllSymbols(); 
        return stringToReplace; 
    } 
 
 
    private void replaceAllSymbols() { 
        for (String symbolName = symbolIterator.nextSymbol(); symbolName != null; symbolName = symbolIterator.nextSymbol()) 
            replaceAllInstances(symbolName); 
    } 
 
    private void replaceAllInstances(String symbolName) { 
        if (shouldReplaceSymbol(symbolName)) 
            replaceSymbol(symbolName); 
    } 
 
    private void replaceSymbol(String symbolName) { 
        alreadyReplaced.add(symbolName); 
        stringToReplace = stringToReplace.replace( 
                symbolTranslator.symbolExpression(symbolName), 
                symbolTranslator.getSymbol(symbolName)); 
    } 
 
    private boolean shouldReplaceSymbol(String symbolName) { 
        return symbolTranslator.getSymbol(symbolName) != null && !alreadyReplaced.contains(symbolName); 
    } 
 
}

Notice that replaceSymbol  makes two calls to symbolTranslator, I felt there was a case of feature envy going on here. So next up I dealt with that by adding a translate method to symbolTranslator, the symbolExpression method was no longer called from SymbolReplacer so that could be made private.

SymbolTranslator.java
/** 
 * Created by dave on 14/05/15. 
 */ 
public abstract class SymbolTranslator { 
 
 
    public static final String SYMBOL_PREFIX = "$"; 
 
    public String translate(String stringToReplace, String symbolName) { 
        return stringToReplace.replace( 
                symbolExpression(symbolName), 
                getSymbol(symbolName)); 
    } 
 
    public boolean hasTranslation(String symbolName) { 
        return getSymbol(symbolName) != null; 
    } 
 
    protected abstract String getSymbol(String symbolName); 
 
    private String symbolExpression(String symbolName) { 
        return SYMBOL_PREFIX + symbolName; 
    } 
 
} 


SymbolReplacer.java
import java.util.ArrayList; 
import java.util.List; 
import java.util.regex.Matcher; 
import java.util.regex.Pattern; 
 
class SymbolReplacer { 
    private final SymbolIterator symbolIterator; 
    protected String stringToReplace; 
    private final SymbolTranslator symbolTranslator; 
    protected List alreadyReplaced = new ArrayList(); 
 
    SymbolReplacer(String s, SymbolTranslator symbolTranslator) { 
        this.stringToReplace = s; 
        this.symbolTranslator = symbolTranslator; 
        this.symbolIterator = new SymbolIterator(s); 
    } 
 
    String replace() { 
        replaceAllSymbols(); 
        return stringToReplace; 
    } 
 
 
    private void replaceAllSymbols() { 
        for (String symbolName = symbolIterator.nextSymbol(); symbolName != null; symbolName = symbolIterator.nextSymbol()) 
            replaceAllInstances(symbolName); 
    } 
 
    private void replaceAllInstances(String symbolName) { 
        if (shouldReplaceSymbol(symbolName)) 
            replaceSymbol(symbolName); 
    } 
 
    private void replaceSymbol(String symbolName) { 
        alreadyReplaced.add(symbolName); 
        stringToReplace = symbolTranslator.translate(stringToReplace, symbolName); 
    } 
 
    private boolean shouldReplaceSymbol(String symbolName) { 
        return symbolTranslator.getSymbol(symbolName) != null && !alreadyReplaced.contains(symbolName); 
    } 
 
}

Definitely starting to look simpler I hope you agree. I now felt that the call to getSymbol to see if there was a symbol defined was giving away some implementation details of the SymbolTranslator so moved it across to there. At the same time I collapsed back in some of the private methods to see if another view on things helped now there had been some restructuring.

SymbolReplacer.java
import java.util.ArrayList; 
import java.util.List; 
 
class SymbolReplacer { 
    private final SymbolIterator symbolIterator; 
    protected String stringToReplace; 
    private final SymbolTranslator symbolTranslator; 
    protected List alreadyReplaced = new ArrayList(); 
 
    SymbolReplacer(String s, SymbolTranslator symbolTranslator) { 
        this.stringToReplace = s; 
        this.symbolTranslator = symbolTranslator; 
        this.symbolIterator = new SymbolIterator(s); 
    } 
 
    String replace() { 
        for (String symbolName = symbolIterator.nextSymbol(); symbolName != null; symbolName = symbolIterator.nextSymbol()) 
            replaceAllInstances(symbolName); 
        return stringToReplace; 
    } 
 
 
    private void replaceAllInstances(String symbolName) { 
        if (symbolTranslator.hasTranslation(symbolName) && !alreadyReplaced.contains(symbolName)) { 
            alreadyReplaced.add(symbolName); 
            stringToReplace = symbolTranslator.translate(stringToReplace, symbolName); 
        } 
    } 
 
}

Getting a lot neater. But I still felt the SymbolReplacer was doing two things, it was iterating through all the symbols and ensuring that we didn’t replace them more than once with the alreadyReplaced collection – I wanted to pull this idempotency out so I extracted a further class “IdempotentSymbolReplacer” to deal with this aspect.

IdempotentSymbolReplacer.java
import java.util.ArrayList; 
import java.util.List; 
 
/** 
 * Created by dave on 14/05/15. 
 */ 
public class IdempotentSymbolReplacer { 
 
    private final SymbolTranslator symbolTranslator; 
    private List alreadyReplaced = new ArrayList(); 
 
    public IdempotentSymbolReplacer(SymbolTranslator symbolTranslator) { 
        this.symbolTranslator = symbolTranslator; 
    } 
 
    public String replaceAllInstances(String symbolName, String stringToReplace) { 
        if (symbolTranslator.hasTranslation(symbolName) && !alreadyReplaced.contains(symbolName)) { 
            alreadyReplaced.add(symbolName); 
            return symbolTranslator.translate(stringToReplace, symbolName); 
        } 
        return stringToReplace; 
    } 
 
} 



Final tidy up

After a final tidy up my finished code is below;

SymbolReplacer.java
class SymbolReplacer { 
    private final IdempotentSymbolReplacer idempotentSymbolReplacer; 
    protected String stringToReplace;; 
 
    SymbolReplacer(String stringToReplace, SymbolTranslator symbolTranslator) { 
        this.stringToReplace = stringToReplace; 
        idempotentSymbolReplacer = new IdempotentSymbolReplacer(symbolTranslator); 
 
    } 
 
    String replace() { 
        SymbolIterator symbolIterator = new SymbolIterator(stringToReplace); 
        for (String symbolName = symbolIterator.nextSymbol(); symbolName != null; symbolName = symbolIterator.nextSymbol()) 
            idempotentSymbolReplacer.replaceAllInstances(symbolName, stringToReplace); 
        return stringToReplace; 
    } 
 
 
}
SymbolTranslator.java
/** 
 * Created by dave on 14/05/15. 
 */ 
public abstract class SymbolTranslator { 
 
 
    public static final String SYMBOL_PREFIX = "$"; 
 
    public String translate(String stringToReplace, String symbolName) { 
        return stringToReplace.replace( 
                symbolExpression(symbolName), 
                getSymbol(symbolName)); 
    } 
 
    public boolean hasTranslation(String symbolName) { 
        return getSymbol(symbolName) != null; 
    } 
 
    protected abstract String getSymbol(String symbolName); 
 
    private String symbolExpression(String symbolName) { 
        return SYMBOL_PREFIX + symbolName; 
    } 
 
} 


SymbolIterator.java
import java.util.regex.Matcher; 
import java.util.regex.Pattern; 
 
/** 
 * Created by dave on 14/05/15. 
 */ 
public class SymbolIterator { 
    private Matcher symbolMatcher; 
    private final Pattern symbolPattern = Pattern.compile("\\" + SymbolTranslator.SYMBOL_PREFIX + "([a-zA-Z]\\w*)"); 
 
    public SymbolIterator(String s) { 
        symbolMatcher = symbolPattern.matcher(s); 
 
    } 
 
    public String nextSymbol() { 
        return symbolMatcher.find() ? symbolMatcher.group(1) : null; 
    } 
} 


IdempotentSymbolReplacer.java
import java.util.HashSet; 
import java.util.Set; 
 
/** 
 * Created by dave on 14/05/15. 
 */ 
public class IdempotentSymbolReplacer { 
 
    private final SymbolTranslator symbolTranslator; 
    private Set alreadyReplaced = new HashSet(); 
 
    public IdempotentSymbolReplacer(SymbolTranslator symbolTranslator) { 
        this.symbolTranslator = symbolTranslator; 
    } 
 
    public String replaceAllInstances(String symbolName, String stringToReplace) { 
        if (!symbolTranslator.hasTranslation(symbolName) || alreadyReplaced.contains(symbolName)) 
            return stringToReplace; 
 
        alreadyReplaced.add(symbolName); 
        return symbolTranslator.translate(stringToReplace, symbolName); 
    } 
 
} 


Conclusions

I think this has led to some much simpler code, lower cyclomatic complexity with single purpose methods and classes. I find this a great way to understand a new code base.

Further thoughts are;

  • IdempotentSymbolReplacer still does two things – should we split it down further?
  • It feels like there is a missing abstraction of “Symbol”
  • Could the SymbolReplacer interface be improved to allow multiple calls to replace without constructing a new instance every time?

Remember:

Extract till you drop

then

Abstract till you drop

Thanks to Uncle Bob for his enthusiastic permission to re-use his code here.

Advertisements
Tagged , ,

5 thoughts on “One thing: Abstract till you drop!

  1. Interesting insight that “the code smell is clearer” by creating functions which only do one thing! Unfortunately I don’t have time to check out your refactored code sample, but judging from your definition of “Abstract till you drop” (which by the way sounds way better than my “Always develop towards an API.”), I anticipate it to be a great improvement. I guess I personally just prefer jumping to that final state immediately, and skipping the intermediate ‘function hell’. 😉

    Liked by 1 person

  2. hypesystem says:

    I wrote something about kind of the same thing, but at a more abstract and general level. I call it “Pushing complexity” (can’t seem to be allowed to post a link).

    I loved this post — great read, and I definitely agree!

    Like

  3. thinkfoo says:

    Thanks for the feedback hypesystem’s article is here
    http://blog.hypesystem.dk/pushing-complexity

    Like

  4. Ben says:

    Thanks for writing this. Good stuff. It would be interesting to see an examination of testability of first solution vs. final solution, and maybe even look to see if one is more provably correct – that is, parts of the final solution may be shown to be correct that could not be before. Or not?

    Like

    • thinkfoo says:

      Hi Ben, thanks for your comments. I think it’s probable that where I ended up is more testable. I have certainly thought that had a TDD approach been used in the first place here it’s likely that the solution would be closer to my end position.

      Like

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s

%d bloggers like this: