Database TDD Part 13: Keeping Time

OK, I lied. I’m not tackling building an abstraction for the metadata yet. I’d rather hit duplication first and see what that leads me to.

Concerned about how long I might take to get to a green bar, I put the timer on.

Goal: factor the duplicate save methods into a common representation. The steps in mind:

  1. Create a persister class to contain the common save method
  2. Abstract the metadata elements needed for saving (i.e. the columns and table name) into an interface
  3. Implement the interface in the UserAccess class
  4. From the UserInterface save method, construct a persister with the metadata
  5. Call the save method on the persister, passing the object to be persisted

UserAccess

package domain;

import java.util.*;

import persistence.*;

public class UserAccess implements PersistableMetadata {
   private static final String TABLE = "userdata";
   private static String[] COLUMNS = { User.NAME, User.PASSWORD };

   public void save(Persistable persistable) {
      new Persister(this).save(persistable);
   }

   public User find(String nameKey) {
   ...
   }

   public String getTable() {
      return TABLE;
   }

   public String[] getColumns() {
      return COLUMNS;
   }
}

PersistableMetadata

package domain;

public interface PersistableMetadata {
   String getTable();
   String[] getColumns();
}

Persister

package domain;

import persistence.*;

public class Persister {
   private PersistableMetadata metadata;
   public Persister(PersistableMetadata metadata) {
      this.metadata = metadata;
   }

   public void save(Persistable persistable) {
      String[] columns = metadata.getColumns();
      String[] fields = new String[columns.length];
      for (int i = 0; i < columns.length; i++)
         fields[i] = persistable.get(columns[i]);
      String sql = new SqlGenerator().createInsert(metadata.getTable(), columns, fields);
      new JdbcAccess().execute(sql);
   }

}

Pretty simple. Checking the timer, I’m at a little over 4 minutes, and I have a green bar.

Even though my tests pass, I try to never extract a new class without adding some level of test to it. If I can’t get a reasonable test against the new class, then it suggests something’s awry with my refactoring. Timer on.

package domain;

import persistence.*;
import junit.framework.*;

public class PersisterTest extends TestCase {
   private static final String TABLE = "x";
   private static final String[] COLUMNS = { "a", "b" };
   private String lastSql;

   public void testSave() {
      PersistableMetadata metadata = new PersistableMetadata() {
         public String getTable() {
            return TABLE;
         }

         public String[] getColumns() {
            return COLUMNS;
         }

      };

      Persistable persistable = new Persistable() {
         public String get(String key) {
            if (key.equals(COLUMNS[0]))
               return "0";
            if (key.equals(COLUMNS[1]))
               return "1";
            return null;
         }
      };

      JdbcAccess access = new JdbcAccess() {
         public void execute(String sql) {
            lastSql = sql;
         }
      };

      Persister persister = new Persister(metadata, access);
      persister.save(persistable);

      assertEquals("insert into x (a,b) values ('0','1')", lastSql);
   }
}

Gak. Three anonymous inner class overrides. Who has a problem with that? I’ve come across many developers who just hate anonymous inner classes. Me, I tend to use them all the time when I need to stub something for purposes of a test.

Here, the metadata and persistable dynamic class definitions do something slightly different: they define an invariant class that I can guarantee for purposes of the test. They are no different than any other class I might define. The JdbcAccess override, however, dummies out the execute method, so that I can (a) avoid having to do actual, slow persistence when running the test and (b) prove that Persister is working by testing against the SQL string that would otherwise be executed.

Using the JdbcAccess override does require changes to the Persister class itself. I had to add an additional constructor, and then do something similar to Parameterize Constructor (see Michael Feathers’ book Working Effectively With Legacy Code).

package domain;

import persistence.*;

public class Persister {
   private PersistableMetadata metadata;
   private JdbcAccess access;

   // general use production constructor
   public Persister(PersistableMetadata metadata) {
      this(metadata, new JdbcAccess());
   }

   // new overloaded constructor
   public Persister(PersistableMetadata metadata, JdbcAccess access) {
      this.metadata = metadata;
      this.access = access;
   }

   public void save(Persistable persistable) {
      String[] columns = metadata.getColumns();
      String[] fields = new String[columns.length];
      for (int i = 0; i < columns.length; i++)
         fields[i] = persistable.get(columns[i]);
      String sql = new SqlGenerator().createInsert(metadata.getTable(), columns, fields);
      access.execute(sql); // no longer directly instantiating here
   }

}

Timer check: about 5 minutes (typing fast). Final step for today: refactor CustomerAccess the same way. Timer check: about 45 seconds.

Next: there’s now some obvious duplication in CustomerAccess and UserAccess.

Database TDD Part 12: Domain Maps

Having explicit fields in Java is a source of redundancy–one that I’m usually willing to live with. But in the case of persisting fields from objects, they’re all one and the same kind of thing, so why deal with the nuisance of having them stored in separately named fields? Moving forward, I’ve decided to eliminate this redundancy so that my persistence code can be cheaply maintained.

The first step is to convert the internal representation of the customer fields to a domain map.

package domain;

import junit.framework.*;

public class CustomerTest extends TestCase {
   private Customer customer;

   public void testCreate() {
      final String id = "123";
      final String name = "Smelt Systems";
      customer = new Customer(id, name);
      assertField(id, customer.getId(), Customer.ID);
      assertField(name, customer.getName(), Customer.NAME);
   }

   private void assertField(String expected,
         String actual, String key) {
      assertEquals(key, expected, actual);
      assertEquals(key, expected, customer.get(key));
   }
}

Question: should I write a test against the values map? I intend for it to be exposed at package level access, at least for the time being. I’ll keep the test for now and if it causes any dependency headaches, it’s gone. I think the issue will resolve itself when I get the persistence stuff updated for both domain classes.

package domain;

import java.util.*;

public class Customer {
   static final String ID = "id";
   static final String NAME = "name";
   private Map<String> values = new HashMap<String,String>();

   public Customer(String id, String name) {
      values.put(ID, id);
      values.put(NAME, name);
   }

   public String getId() {
      return values.get(ID);
   }

   public String getName() {
      return values.get(NAME);
   }

   String get(String key) {
      return values.get(key);
   }
}

This raises a couple of flags. First off, I’m still dealing only with String fields, which makes things simpler. Ultimately we’ll see how refactorable this is when I introduce a new type. Second, what of null values? Or what happens if a field is not initialized (i.e. the constructor never puts a value in)?

The first question is easy to resolve:

   public void testNullValues() {
      Customer customer = new Customer(null, null);
      assertNull(customer.getId());
      assertNull(customer.getName());
   }

The test passes. The second question relates to something I don’t care about yet, but maybe it’s a good idea to write a sketch of a test.

   /*
   public void futuretestNulls() {
      Customer customer = new Customer();
      assertNull(customer.getId());
   }
   */

The problem with “reminder” tests like these is that they sometimes stick around forever. I tend to delete them when I see them on principle. Here I might keep it around for my own use since I know that I’m going to hit this code shortly. I’d probably avoid checking in reminder tests like these.

For the keys to the domain map, I deliberately chose the same names as the column names in the customer table. Why not? The only problem I have with this scheme is that it’s a hidden dependency that should somehow be made more explicit. Implications of the dependency are that things can break if a developer is unaware of it; also, the Customer class must recompile if the column names change. I’m ok with the second part. Suggestions on how to properly document the hidden dependency?

Here’s where the fun begins. The save method in CustomerAccess becomes more generic:

public class CustomerAccess {
   private static final String TABLE = "cust";
   private static String[] COLUMNS = { Customer.ID, Customer.NAME };

   public void save(Customer customer) {
      String[] fields = new String[COLUMNS.length];
      for (int i = 0; i < COLUMNS.length; i++)
         fields[i] = customer.get(COLUMNS[i]);
      String sql = new SqlGenerator().createInsert(TABLE, COLUMNS, fields);
      new JdbcAccess().execute(sql);
   }

The only thing specific is the Customer type–I hear an interface calling.

After changing the implementation of User to also be a domain map, and after making the same changes to the UserAccess, I can now start to factor out the duplication. Both Customer and User have the values map. I could have them both inherit from a common class, but I’m very reluctant to introduce inheritance, since you get one shot. For now, I’m going to concentrate on the duplication in the save method, and use an interface to define the commonality between Customer and User.

package domain;

public interface Persistable {
   String get(String key);
}

Now the save method can be written as:

   public void save(Persistable persistable) {
      String[] fields = new String[COLUMNS.length];
      for (int i = 0; i < COLUMNS.length; i++)
         fields[i] = persistable.get(COLUMNS[i]);
      String sql = new SqlGenerator().createInsert(TABLE, COLUMNS, fields);
      new JdbcAccess().execute(sql);
   }

… in both UserAccess and CustomerAccess.

I can now either create a new class that contains the save method, and delegate from the access objects. Or I can push up the method into a common superclass. Either solution requires a bit of work. The problem with pushing up the method is that it refers to the subclass-specifc TABLE and COLUMNS fields. I could push that information into abstract method overrides, and set save up to be a template method.

If I go the delegation route, the new class will need to be passed the pertinent metadata as well as the persistable object. I’m tending to go this route, due to my resistance of inheritance (particularly at this stage in the game). Right now, the code is screaming out for some sort of abstraction to represent the metadata; I think that’s what I’m going to tackle next.

Taking a Break?

Instead of working on the database code today, I concentrated on trying to get my web site back to working state. I made some (live!) efforts last night toward putting more CSS-driven layout onto the site. I learned HTML nine years ago, and was still using tables to do my layout. That’s what I knew; I’d not taken the time to pick up CSS layout.

I almost broke down and bought a book on CSS. The people who do web site design are good at what they do, but what they’re usually not good at is explaining it. I had difficulty finding a site that clearly stepped me through what I was trying to do. But I finally found one this morning:this page at 456 Berea Street talks about building a simple two-column page layout. And it does it in terms that I can understand. Thanks, whoever wrote that.

My site has been a bit of a disaster since yesterday. I’ve been doing much of this live, and at one point chose to forge ahead with a significant defect instead of backing it out. The site has finally settled down, but I’m sure there are all sorts of little defects. I’ll work at cleaning them up one by one. My favorite part right now is the errata page, where I replaced a table with a definition list (dl/dt/dd tags). I’m pretty impressed with how much this stuff can improve the look.

I’m sure there are all sorts of defects Internet Explorer exposes that Firefox does not. I came across at least three while I was working, and figuring out how to resolve them was more pain than I would have liked. It turns out that IE is probably the main source of defects, not my code. On at least one page, the text would actually disappear when you scrolled around. Apparently there are a number of workarounds for these IE bugs.

Despite a rough start, I’m finally feeling somewhat comfortable about doing CSS-based layout. It’s not always obvious, but at least it’s starting to make more sense. It has also considerably cleaned up some of my messier pages (nightmares of tables within tables). For that I’m thankful.

Database TDD Part 10: Performance

It’s amazing how many times I find myself violating my own rules, my own guidelines, the things I’ve preached over the years. Performance is often one of them. Knowing performance to be critical when it comes to database access, I’ve prematurely optimized one of the String utility methods. One of my invisible pairs (people commenting on these blogs) pointed out that my use of StringBuilder has unnecessarily obfuscated code in a couple spots.

StringUtil

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();
   }
}

The wrap and commaDelimit methods could both be written using String concatenation (+) instead of StringBuilder. Habitually, I coded this using a StringBuilder. Now, I’ve preached against this unnecessary optimization for years, including in Essential Java Style. So why did I violate it?

If I were in court, I’d plead nolo contendere. The wrap method is far more succinctly written as:

   public static String wrap(String source, char wrapper) {
      if (source == null)
         return null;
      return wrapper + source + wrapper;
   }

Using a StringBuilder in this method significantly detracted from its readability. Guilty as charged. With respect to thecommaDelimit method, I plead legitimate past experience. The general rule of thumb is that String concatenation is fine when putting together a finite, reasonable number of Strings. When using looping constructs, where each iteration through the loop appends to the previous complete string (which starts empty), the performance degradation can be significant.

But really, you don’t know until you’ve measured it. The rule of performance is to only optimize after you’ve gotten things working properly, and only then if you need to. I do counterbalance that with a second rule: it’s ok to optimize if it does not detract from the readability or future understanding of the code. By “future understanding,” I’m talking about when someone looks at the code and has to say “why the heck would he do it that way?.” That’s not a good thing.

Consider a rewrite of the commaDelimit method:

   public static String commaDelimit(String[] strings, Transformer transformer) {
      String builder = "";
      for (int i = 0; i < strings.length; i++) {
         if (i > 0)
            builder += ',';
         builder += transformer.transform(strings[i]);
      }
      return builder.toString();
   }

In my humble opinion, that adds virtually nothing to its readability. Replacing a method call with a symbol is nice, but doesn’t really change much with respect to the time required to comprehend commaDelimit.

For now, I’m going to straddle the fence. I’ve already rewritten the wrap method. I intend on keeping the existing definition ofcommaDelimit, which, by the way, has another potential performance issue in the callback to the transformer. Performance debates never end. The best way to shut them off is to write a quick performance test. If someone else is concerned about performance, let me know, and we’ll tackle that.

ps – I did write the performance tests for commaDelimit. It’s from three to ten times slower when written using String concatenation. I also wrote the tests for wrap. It’s not quite twice as slow now, and I had to ratchet up the number of iterations to 100,000 before it even got into the realm of milliseconds. Right now, these measurements are meaningless.

Original Comments:

Using buffer/builders for looping constructs is what I would have done as well.

–JeffBay

Database TDD Part 11: Blatant Duplication

I always waver on the decision between creating duplication and then eliminating it, or staving it off immediately before you would have created it. I think the former is safer in terms of coming up with a better solution. It is, however, slower, something we’ll see today.

I wanted to force the issue of duplication by adding a second domain class, so as to figure out how the persistence driver was going to shake out. I created a new class Customer along with its associated test class. Copy and paste, literally. It’s a stupid data class, but let’s pretend it has legitimate behavioral capabilities that forced it into existence. The customer class contains an id and a name, both strings.

Same thing for Customer persistence needs: new CustomerAccessTest and CustomerAccess classes are pretty much copy-and-pasted. Is copying and pasting code bad? In my view, nothing’s bad until it’s checked in. Here it’s a tool to make the duplication explicit, visible in all its gory details, right in front of my face.

The alternative would be to factor relevant code from the UserAccess class. But I find it slightly more difficult to try to put all of that into my head. Here I can start with the obvious duplication and slowly refactor to an appropriate solution.

The first refactoring is to extract code that generates SQL into a completely separate class.

public class UserAccess {
   private static final String TABLE_NAME = "userdata";
   private static String[] columns = { "name", "password" };

   public void save(User user) {
      String sql = new SqlGenerator().createInsert(TABLE_NAME, columns, createValuesList(user));
      new JdbcAccess().execute(sql);
   }

   private String createValuesList(User user) {
      Transformer ticWrapper = new Transformer() {
         public String transform(String input) {
            return StringUtil.wrap(input, '\'');
         }
      };
      return StringUtil.commaDelimit(new String[] { user.getName(),
            user.getPassword() }, ticWrapper);
   }
   ...
package persistence;

import util.*;

public class SqlGenerator {
   public String createInsert(String tableName, String[] columns, String values) {
      return String.format("insert into %s (%s) values (%s)", tableName,
            createColumnList(columns), values);
   }

   private String createColumnList(String[] columns) {
      return StringUtil.commaDelimit(columns);
   }
}

The createValuesList method should obviously belong to in the SqlGenerator class. The problem is how to get the user fields to the SqlGenerator class such that it doesn’t create a backward dependency. Easy enough: prepopulate the array of field values and pass it over.

   // UserAccess code
   public void save(User user) {
      String[] fields = { user.getName(), user.getPassword() };
      String sql = new SqlGenerator().createInsert(TABLE_NAME, columns, fields);
      new JdbcAccess().execute(sql);
   }
package persistence;

import util.*;

public class SqlGenerator {
   public String createInsert(String tableName, String[] columns, String[] fields) {
      return String.format("insert into %s (%s) values (%s)", tableName,
            createColumnList(columns), createValuesList(fields));
   }

   private String createColumnList(String[] columns) {
      return StringUtil.commaDelimit(columns);
   }

   private String createValuesList(String[] fields) {
      Transformer ticWrapper = new Transformer() {
         public String transform(String input) {
            return StringUtil.wrap(input, '\'');
         }
      };
      return StringUtil.commaDelimit(fields, ticWrapper);
   }
}

The rest of the code refactoring is pretty straightforward, based on this direction. When I finished, my CustomerAccess class looked like this:

package domain;

import java.util.*;

import persistence.*;

public class CustomerAccess {
   private static final String TABLE = "cust";
   private static String[] COLUMNS = { "id", "name" };

   public void save(Customer customer) {
      String[] fields = { customer.getId(), customer.getName() };
      String sql = new SqlGenerator().createInsert(TABLE, COLUMNS, fields);
      new JdbcAccess().execute(sql);
   }

   public Customer find(String idKey) {
      String sql = new SqlGenerator().createFindByKey(TABLE, COLUMNS, COLUMNS[0], idKey);
      JdbcAccess access = new JdbcAccess();
      List<String> row = access.executeQuery(sql);
      return new Customer(row.get(0), row.get(1));
   }
}

This looks a like nicer, but it also still looks way too much like UserAccess. The fact that data is stored on the domain objects in explicit fields is going to continue to be a nuisance–unless I do something about that.

I’m essentially building metadata here, about the database tables and the domain objects for which they hold data. Where are these tables coming from? Right now, the presumption is that they already exist. What if I were to define the tables myself from code? Or what about the opposite–obtain all the appropriate data from the database itself? There are myriad ways to proceed from here; I’m going to need to decide on a direction soon. I’m willing to go in any direction–your input is welcomed.

For now, I’m going to finish cleanup on the code base, making some simple name changes and adding tests where needed.

Original Comments:

My rule of thumb is to factor before introducing duplication if I can visualize in my head what it should look like after and I like the way it will look. If I can’t, then I duplicate it and then figure out how to remove the duplication.

Sometimes, I choose the former approach, and I’m wrong about my ability to visualize it, or I saw it incorrectly – if I can’t get back to passing test quickly, I start looking for a way to get back to passing tests by falling back to the duplicate first strategy.

I think you can save time and produce good solutions by factoring before adding behaviour – but there is a balance in how far you take it.

— JeffBay

And I’m not invisible, I’ve been signing my posts! :p Anonymous : 10/27/2005 09:13:00 AM

Database TDD Part 9: Single Responsibility Principle

The User class is still doing too much by managing its own persistence. Following the SRP, I’ve redesigned the classes so that the User class only manages user information, and a new class, UserAccess, persists users. In the new design, the User class knows nothing about the persistence mechanism whatsoever. This does change the client interface, mind you.

UserTest

package domain;

import junit.framework.*;

public class UserTest extends TestCase {
   static final String name = "a";
   static final String password = "b";
   private User user;

   protected void setUp() {
      user = new User(name, password);
   }

   public void testCreate() {
      assertEquals(name, user.getName());
      assertEquals(password, user.getPassword());
   }
}

User

package domain;

public class User {
   private String name;
   private String password;

   public User(String name, String password) {
      this.name = name;
      this.password = password;
   }

   public String getName() {
      return name;
   }

   public String getPassword() {
      return password;
   }
}

UserAccessTest

package domain;

import junit.framework.*;

public class UserAccessTest extends TestCase {
   public void testPersist() {
      final String name = "a";
      final String password = "b";

      User user = new User(name, password);
      UserAccess access = new UserAccess();
      access.save(user);
      User retrievedUser = access.find(name);
      assertEquals(name, retrievedUser.getName());
      assertEquals(password, retrievedUser.getPassword());
   }
}

UserAccess

package domain;

import java.util.*;

import persistence.*;
import util.*;

public class UserAccess {
   private static final String TABLE_NAME = "userdata";
   private static String[] columns = { "name", "password" };

   public void save(User user) {
      new JdbcAccess().execute("insert into " + TABLE_NAME + " ("
            + createColumnList()  + ") values ("
            + createValuesList(user) + ")");
   }

   private String createValuesList(User user) {
      Transformer ticWrapper = new Transformer() {
         public String transform(String input) {
            return StringUtil.wrap(input, '\'');
         }
      };
      return StringUtil.commaDelimit(new String[] { user.getName(),
            user.getPassword() }, ticWrapper);
   }

   public User find(String nameKey) {
      JdbcAccess access = new JdbcAccess();
      List<String> row = access.executeQuery(String.format(
            "select " + createColumnList() + " from " + TABLE_NAME + " where name = '%s'",
            nameKey));
      return new User(row.get(0), row.get(1));
   }

   private static String createColumnList() {
      return StringUtil.commaDelimit(columns);
   }
}

A minor refactoring followed. I don’t recall how I got there, but my use of the String format method (introduced in J2SE 5.0) is less than ideal. The save and find methods can be cleaned up so that the SQL strings are much easier to read.

   public void save(User user) {
      String sql = String.format("insert into %s (%s) values (%s)", TABLE_NAME,
            createColumnList(), createValuesList(user));
      new JdbcAccess().execute(sql);
   }
...
   public User find(String nameKey) {
      String sql = String.format("select %s from %s where name = '%s'",
            createColumnList(), TABLE_NAME, nameKey);
      JdbcAccess access = new JdbcAccess();
      List&lt;String&gt; row = access.executeQuery(sql);
      return new User(row.get(0), row.get(1));
   }

So far, nothing earth shattering. The more interesting challenge is up next: we have to persist objects of a second type. The imminent redundancy should already be apparent. How will we keep UserAccess and whatever new access class from looking almost exactly alike?

Database TDD Part 8: Zealotry

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.

Original Comments:

 

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.

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.

 

Database TDD Part 7: Listening to Your Pair

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<String> 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:

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

Database TDD Part 6: Duplication in SQL

This morning: a five-minute refactoring to further eliminate duplication in the code. SQL statements are inherently redundant. They’re also risky to put together without a test against a live database.

Let’s hit the code. Here’s the entire User class:

import java.util.*;

public class User {
   private String name;
   private String password;
   private static final String TABLE_NAME = "userdata";
   private static String[] columns = { "name", "password" };

   public User(String name, String password) {
      this.name = name;
      this.password = password;
   }

   public String getName() {
      return name;
   }

   public String getPassword() {
      return password;
   }

   public void save() {
      new JdbcAccess().execute(String.format("insert into " + TABLE_NAME + " ("
            + User.createColumnList() + ") values ('%s', '%s')", name, password));
   }

   private static String createColumnList() {
      StringBuilder builder = new StringBuilder();
      for (int i = 0; i &lt; columns.length; i++) { if (i &gt; 0)
            builder.append(',');
         builder.append(columns[i]);
      }
      return builder.toString();
   }

   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));
   }
}

This was three refactoring passes, each concluded with the execution of all tests in the project. The first pass involved extracting the table name to a constant; 30 seconds. The second pass involved extracting the construction of the insert column list to a separate method that uses a String array of column names; 4 minutes. The third pass involved using createColumnList from the find method; 30 seconds.

The table name and the column lists were an obvious place to start. What about the values list in the insert statement? What about the duplication inherent in the User attributes themselves?

Another thought: the Single Responsibility Principle is getting more and more abused. It’s bad enough that the User domain class deals with persistence. Now there’s a new method createColumnList that is a generic String utility method. It belongs elsewhere. But don’t fret, we’ll get to that soon.

Original Comments:

Speaking of the single responsibility principle, how long until you make first class objects for Column and Table, and fold some of the sql creation logic into them?

–JeffBay

===
And on another note, instead of “createColumnList”, or in addition to, the more generic “join” method would be a great addition to your string utilities handbook.

Strings.join(String[] strings, String delimeter)

createColumnList(columns) {
return Strings.join(columns, “, “);
}

–JeffBay

===
Good comments, both. Not long. What I find interesting is that in 10 minutes worth of initial DB code, there’s so much refactoring that can be done.

Tonight, though, I’m almost asleep already. Maybe tomorrow night.

-j-

Database TDD Part 5: Encapsulating JDBC

You want your persistence layer to not leak its implementation details into the rest of your application. Most systems make this mistake, however: the fact that SQL via JDBC is being used is explicit in a large number of classes. Changing to a new persistence solution, such as JDO, CMP EJBs, or Hibernate, becomes a major undertaking.

Even if you’re careful to isolate all your JDBC calls to a single class, as I’m trying to do, it’s easy to ooze JDBC-specific code into the rest of your app. The culprit is java.sql.SQLException. Any code that must acknowledge this exception is now dependent on JDBC.

An easy solution would be to throw the more abstract and palatable type, Exception. But I prefer just to not propagate checked exceptions. They are a nuisance. “Inbetween” code rarely can do anything with them; top-level code can usually only report them through the user interface. Developers often lazily catch them and log an error that no one discovers until some insidious defect has escaped. For all the promises of checked exceptions, they tend to do little more than force littering of the code with try/catch blocks and throws clauses. Checked exceptions cause unnecessary duplication.

For the JdbcAccess class, then, I’ve chosen to propagate a runtime exception of a custom type. It could be of type RuntimeException, but using a custom type imparts some immediately useful information.

Driving the custom runtime exception through tests, I end up with a couple tests that look much alike. Here’s one of them:

   public void testExecuteException() {
      try {
         access.execute(BADLY_FORMED_SQL);
         fail(FAILURE_MESSAGE);
      }
      catch (JdbcException e) {
         assertException(e);
      }
   }

   private void assertException(JdbcException e) {
      assertEquals(BADLY_FORMED_SQL, e.getMessage());
      assertTrue(e.getCause() instanceof java.sql.SQLException);
   }

The other test is for executeQuery. The exception class:

public class JdbcException extends RuntimeException {
   public JdbcException(String sql, Throwable cause) {
      super(sql, cause);
   }
}

As I added the first exception test to the JdbcAccessTest class that I had from yesterday, I realized that the tearDown method deleted a sample table created solely for purposes of the test. The exception tests have no such need for a table, and thus don’t create one. This meant that the tearDown method would now throw an exception when it tried to drop a nonexistent table.

Rather than rework existing JdbcAccessTest code, the most straightforward solution is to create another test class. There’s no rule that says you can only have a single test class for each production class. Each test class can be treated as a separate fixture, with its own context that must be set up and possibly destroyed. Here’s the new test class in its entirety:

import junit.framework.*;

public class JdbcAccessExceptionsTest extends TestCase {
   private JdbcAccess access;
   private static final String BADLY_FORMED_SQL = "badly formed sql";
   private static final String FAILURE_MESSAGE = "expected exception from malformed sql";
   protected void setUp() {
      access = new JdbcAccess();
   }

   public void testExecuteException() {
      try {
         access.execute(BADLY_FORMED_SQL);
         fail(FAILURE_MESSAGE);
      }
      catch (JdbcException e) {
         assertException(e);
      }
   }

   public void testExecuteQueryException() {
      try {
         access.executeQuery(BADLY_FORMED_SQL);
         fail(FAILURE_MESSAGE);
      }
      catch (JdbcException e) {
         assertException(e);
      }
   }

   private void assertException(JdbcException e) {
      assertEquals(BADLY_FORMED_SQL, e.getMessage());
      assertTrue(e.getCause() instanceof java.sql.SQLException);
   }
}

The modifications to the production JdbcAccess code involve simply adding try/catch blocks to the public methods.

   public void execute(String sql) {
      try {
         createStatement();
         statement.execute(sql);
         closeConnection();
      }
      catch (SQLException e) {
         throw new JdbcException(sql, e);
      }
   }

   public List&lt;String&gt; executeQuery(String sql) {
      try {
         createStatement();

         ResultSet results = statement.executeQuery(sql);
         results.next();
         List&lt;String&gt; row = getRow(results);
         results.close();

         connection.close();
         return row;
      }
      catch (SQLException e) {
         throw new JdbcException(sql, e);
      }
   }

Don’t forget the best part: going back to UserTest, User, and JdbcAccessTest and eliminating all the throws SQLException clauses plus the importstatement. Your IDE should be able to help you here.

You’ll still need an application-level strategy for managing exceptions at the UI level. Best that you discuss this strategy with your team, and come up with the rules that everyone must play by. One of those rules is that tests should always demonstrate that exceptions can arise.

Atom