I had a different direction in mind for my next increment on the database code. A comment by Jeff B. on the last blog acted as a prod from my virtual pair. When there are a lot of alternatives as to where to proceed next, as there are in this case, I’m more than happy to take a suggestion and run with it.

Jeff suggested something I had in mind but wasn’t going to worry about for a little while. The User class contains a method that creates a column list for an SQL statement. Really, this is a generic string method that concatenates a bunch of strings into a comma-separated list. As such, it’s better suited to being refactored to a common string utility class. That will highlight its availability to other parties on the team, increasing its likelihood of being reused.

This time, instead of refactoring the code to a new class in the absence of tests, I chose to construct the string utility using tests as my driver. I wrote a more complete gamut of tests, including the degenerate case (no strings in the list), one string in the list, and multiple strings in the list. Each time, where appropriate, I simply copied code over from the source method (createColumnList) in the User class. Here’s the new StringUtilTest and StringUtil classes:

    import junit.framework.*;
    
    public class StringUtilCommaSeparateTest extends TestCase {
       public void testDegenerate() {
          assertEquals("", StringUtil.commaSeparate(new String[] {}));
       }
    
       public void testOneEntry() {
          assertEquals("a", StringUtil.commaSeparate(new String[] {"a"}));
       }
    
       public void testTwoEntries() {
          assertEquals("a,b", StringUtil.commaSeparate(new String[] {"a", "b"}));
       }
    
       public void testEmbeddedCommas() {
          // not very well defined yet! don't care so far.
          assertEquals("a,,,b", StringUtil.commaSeparate(new String[] {"a,", ",b"}));
       }
    }

    public class StringUtil {
       public static String commaSeparate(String[] strings) {
          StringBuilder builder = new StringBuilder();
          for (int i = 0; i < strings.length; i++) {
             if (i > 0)
                builder.append(',');
             builder.append(strings[i]);
          }
          return builder.toString();
       }
    }

You’ll note that there’s an additional test, testEmbeddedCommas. This test passed automatically. It’s there as a placeholder. I know that I don’t explicitly handle embedded commas, and that the current definition is less than desirable. But I did think of this possibility. Rather than define something that might not be the correct definition, I chose to leave the implementation as is. The test is there as documentation that the definition may need to be reconsidered in the future.

The test class name isn’t simply “StringUtilTest.” I didn’t want one single test method for commaSeparate with all four asserts in them. I also wanted to avoid the duplication in naming four tests “testCommaSeparateOneEntry,” “testCommaSeparateMultipleEntries,” etc. So I chose to instead recognize these four tests as belonging to their own separate test class (fixture). Future StringUtil tests would go in another fixture. (Begs the question, though, why not just name the class CommaSeparator?)

Testing static methods introduces another form of duplication nuisance. Having to scope the static method call with the class name each time seems unnecessary–particularly if all tests for a single static method appear in one fixture, and the scope is clear. J2SE 5.0 gives you the option of using static import. I don’t recommend the use of static import in many cases, but this is one of the rare cases where its use doesn’t obscure things.

Unfortunately, I just found out that you can’t statically import a class from the default package! Or if you can, I can’t figure out how (any help?). So my next increment will probably involve moving all classes into explicit packages, and then applying the static import.

The final piece of refactoring for the day is the fun part–eliminating the code from User. Since the createColumnList method is called from two places, I chose to keep this method in place, and have it delegate to StringUtil.

    import java.util.*;
    
    public class User {
       ...
       private static String[] columns = { "name", "password" };
       ...
       public void save() {
          new JdbcAccess().execute(String.format("insert into " + TABLE_NAME + " ("
                + User.createColumnList() + ") values ('%s', '%s')", name, password));
       }
    
       private static String createColumnList() {
          return StringUtil.commaSeparate(columns);
       }
    
       public static User find(String nameKey) {
          JdbcAccess access = new JdbcAccess();
          List row = access.executeQuery(String.format(
                "select " + createColumnList() + " from " + TABLE_NAME + " where name = '%s'",
                nameKey));
          return new User(row.get(0), row.get(1));
       }
    }

I think I mentioned this before, but it should now be even more clear that the values list in the insert statement is a comma-separated list. That’s also something I want to factor out… next(?) time. It’s not quite automatic.

Final comment: some of the XP diehards are probably screaming “yagni” (“you ain’t gonna need it”). I can see how it might be claimed–I probably don’t need a test for either the degenerate case or the embedded comma case. (I also don’t really need to extract the string utility code to a separate class, for that matter.) So, is my supposed abuse of YAGNI a good thing or a bad thing?

Comments

Anonymous October 21, 2005 at 02:15am

I say, single responsibility principle and strong tests to communicate the behaviour of degenerate cases trumps yagni every day of the week, including dollarsday.

Share your comment