Some people purport that you can keep the “cost of change” curve relatively flat. In other words, you can introduce unplanned features any time in the future as cheaply as if you had inserted them today. That’s a bold statement. I agree with it if and only if you refactor with extreme vigilance.
To be successful, you will need to become a refactoring zealot. This means you learn to understand what duplication really means, and ferret it out at every possible opportunity. Every TDD cycle includes writing test code, writing production code, then refactoring. Any new code introduced in the first two parts of the cycle must not degrade the system’s current clean design. Otherwise you cannot claim that the cycle is complete.
With the 10-minute bit of code I wrote to support saving and retrieving users, I’ve already spotted gobs of duplication and expressiveness problems. As I refactor more, I seem to spot even more. It’s like clearing the underbrush beneath trees in your yard. Picking up the larger branches on top alerts you to the presence of the smaller branches snaking through the grass. They too have to be cleaned up if you want to keep your grass looking pristine.
Yesterday I mentioned my desire to use static import. That was my first
subject of refactoring today. I moved all of the classes into
appropriate packages. I even renamed a few classes (plus one method),
and at least one class I renamed more than once! I initially moved
JdbcAccess into a package called jdbc, then renamed it to Access (to
remove the redundancy between the class name and the package). But then
I figured I’d be better off with the generic package name persistence,
so I renamed the class back to JdbcAccess. A bit of unfortunate
thrashing, indeed, but it was a cheap exercise. The beauty of Eclipse
and other IDEs is that you don’t have to have a perfect name first. You
can continually improve identifier names as you discover more about what
they represent. Very powerful! This makes the third simple design rule
(code should be expressive, basically) easy and even fun to tackle.
The new project organization:
-
domain.User
-
domain.UserTest
-
persistence.JdbcAccess
-
persistence.JdbcAccessTest
-
persistence.JdbcAccessExceptionsTest
-
persistence.JdbcException
-
util.StringUtil
-
util.StringUtilCommaDelimitTest
I still don’t like some of these class names. Suggestions?
So now, I could use the static import facility:
package util;
import junit.framework.*;
import static util.StringUtil.commaDelimit;
public class StringUtilCommaDelimitTest extends TestCase {
public void testDegenerate() {
assertEquals("", commaDelimit(new String[] {}));
}
public void testOneEntry() {
assertEquals("a", commaDelimit(new String[] {"a"}));
}
public void testTwoEntries() {
assertEquals("a,b", commaDelimit(new String[] {"a", "b"}));
}
public void testEmbeddedCommas() {
// not very well defined yet! don't care so far.
assertEquals("a,,,b", commaDelimit(new String[] {"a,", ",b"}));
}
}
I think this is a perfect use of the (relatively) new J2SE 5.0 feature. Eclipse italicizes the method call, so it’s clear that it’s a static method.
The next part of today’s work (I was only up to about 3 minutes worth of refactoring at this point) was eliminating the duplication inherent in putting together a value list for the insert statement. Step one was to do an extract method refactoring to make it clear what was getting cleaned up. In User:
...
public void save() {
new JdbcAccess().execute("insert into " + TABLE_NAME + " ("
+ User.createColumnList() + ") values (" +
createValuesList() + ")");
}
private String createValuesList() {
return String.format("'%s', '%s'", name, password);
}
...
Step two: enhance my string utility method, looking for duplication along the way. There was lots of it. Here’s what I ended up with:
User.java
public void save() {
new JdbcAccess().execute("insert into " + TABLE_NAME + " ("
+ User.createColumnList() + ") values (" +
createValuesList() + ")");
}
private String createValuesList() {
Transformer ticWrapper = new Transformer() {
public String transform(String input) {
return StringUtil.wrap(input, '\'');
}};
return StringUtil.commaDelimit(new String[] { name, password }, ticWrapper);
}
private static String createColumnList() {
return StringUtil.commaDelimit(columns);
}
StringUtilCommaDelimitTest.java
package util;
import junit.framework.*;
import static util.StringUtil.commaDelimit;
public class StringUtilCommaDelimitTest extends TestCase {
...
public void testCommaDelimitWithCallback() {
Transformer duplicator = new Transformer() {
public String transform(String input) {
return input + input;
}};
assertEquals("aa,bb", commaDelimit(new String[] {"a", "b"}, duplicator));
}
}
Transformer.java
package util;
public interface Transformer {
String transform(String input);
}
StringUtilWrapTest.java
package util;
import junit.framework.*;
import static util.StringUtil.wrap;
public class StringUtilWrapTest extends TestCase {
public void testDegenerate() {
assertNull(wrap(null, '\''));
}
public void testSingleCharacterString() {
assertEquals("*a*", wrap("a", '*'));
}
public void testMultipleCharacterString() {
assertEquals("-abc-", StringUtil.wrap("abc", '-'));
}
}
StringUtil.java
package util;
public class StringUtil {
public static String commaDelimit(String[] strings, Transformer transformer) {
StringBuilder builder = new StringBuilder();
for (int i = 0; i < strings.length; i++) {
if (i > 0)
builder.append(',');
builder.append(transformer.transform(strings[i]));
}
return builder.toString();
}
public static String commaDelimit(String[] strings) {
final Transformer nullTransform = new Transformer() {
public String transform(String input) {
return input;
}
};
return commaDelimit(strings, nullTransform);
}
public static String wrap(String source, char wrapper) {
if (source == null)
return null;
StringBuilder builder = new StringBuilder();
builder.append(wrapper);
builder.append(source);
builder.append(wrapper);
return builder.toString();
}
}
Interestingly, eliminating duplication has meant more code up to this point.
Also, it’s inevitable that you’ll have duplication at some level. Here
it exists in the wrap method. I’ve chosen to foist the duplication
from a volatile area (code in User) to the most abstract level possible,
this wrap method. I don’t see this method as ever having to change.
There’s still SQL-related code in the domain class. That might be my next step, unless someone has another thought.
Comments
Anonymous October 21, 2005 at 12:44pm
You are using StringBuilders a lot in these exercises. Given that they are a lot less readable than string concatenation, it seems like a performance optimization to me. In point of fact, the implementation of the wrap method should be just as efficient written as
return wrapper + string + wrapper;
because of compiler optimization.
Anonymous October 21, 2005 at 12:45pm
sorry, that should have been
return string == null ? null : wrapper + string + wrapper;
I prefer the guard clause when it comes to something like this. While I don’t mind using the ternary operator at times, I’m picky about when I introduce it. Here I think it just makes things a bit more confusing.
