Database TDD Part 18: New Types

So far the database layer supports only strings. That’s actually one way to go–persist everything in the database as strings, and parse the information upon retrieval. I’m not going to do that, although I’ve done it before and it works. It really can get people upset. Instead I’ll build a mechanism to managing mapping between Java types and SQL representation of those types.

To drive out the change, I’ll first update the Customer class with the ability to store a balance, based on charges made against the customer’s account:

CustomerTest

...
public class CustomerTest extends TestCase {
   private Customer customer;
   private static final String ID = "123";
   private static final String NAME = "Smelt Systems";

   protected void setUp() {
      customer = new Customer(ID, NAME);
   }

   public void testCreate() {
      assertField(ID, customer.getId(), Customer.ID);
      assertField(NAME, customer.getName(), Customer.NAME);
   }

   public void testBalance() {
      assertEquals(0, customer.getBalance());
      customer.charge(100);
      assertEquals(100, customer.getBalance());
      customer.charge(500);
      assertEquals(600, customer.getBalance());
   }
   ...
}

Customer

package domain;

import java.util.*;

import persistence.*;

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

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

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

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

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

   public int getBalance() {
      return (Integer)values.get(BALANCE);
   }

   public void charge(int amount) {
      values.put(BALANCE, getBalance() + amount);
   }
}

In Customer, as the simplest solution, I change the values map to support any type of value, instead of just String values. There now appears to be some lack of symmetry between the getBalance and charge methods. I figure there will be some code commonality when dealing with numeric values, but I’ll wait to see what that looks like.

A modified CustomerAccessTest leads to a quick attempt at what the solution might look like from the CustomerAccess side of the fence.

CustomerAccessTest

...
public class CustomerAccessTest extends TestCase {
   public void testPersist() {
      final String name = "a";
      final String id = "1";
      final int amount = 100;

      Customer customer = new Customer(id, name);
      customer.charge(amount);
      CustomerAccess access = new CustomerAccess();
      access.save(customer);
      Customer retrievedCustomer = access.find(id);
      assertEquals(id, retrievedCustomer.getId());
      assertEquals(name, retrievedCustomer.getName());
      assertEquals(amount, retrievedCustomer.getBalance());
   }
}

CustomerAccess

...
public class CustomerAccess extends DataAccess&lt;Customer&gt; {
   ...
   public String[] getColumns() {
      return new String[] { Customer.ID, Customer.NAME, Customer.BALANCE };
   }
   ...
   public Customer create(List<String> row) {
      Customer customer = new Customer(row.get(0), row.get(1));
      customer.charge(Integer.parseInt(row.get(2)));
      return customer;
   }
}

The change to the CustomerAccess create method spews badly. The problem is that the get method returns a string. In fact, code all over the place assumes persisted values are always strings. The core of the problem is in JdbcAccess. Currently, a results row is bound to String. My solution for now is to bind it to the Object type, and worry about type mapping at a higher level.

JdbcAccessTest

...
public class JdbcAccessTest extends TestCase {
   ...
   public void testExecuteQuery() {
      access.execute(createTableSQL());
      List<Object> row = access.executeQuery(createCountSQL());
      assertEquals(1, row.size());
      assertEquals(0, getLong(row, 0));
   }
   ...
   private long count() {
      List<Object> row = access.executeQuery(createCountSQL());
      return getLong(row, 0);
   }
   ...
   private long getLong(List<Object> row, int column) {
      return (Long)row.get(column);
   }
...
}

I also recognized that count should have been defined as a long all along.

JdbcAccess

...
public class JdbcAccess {
   ...
   public List<Object> executeQuery(String sql) {
      try {
         createStatement();

         ResultSet results = statement.executeQuery(sql);
         List<Object> row = null;
         if (results.next())
            row = getRow(results);
         results.close();

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

   private List<Object> getRow(ResultSet results) throws SQLException {
      List<Object> row = new ArrayList<Object>();
      for (int i = 1; i <= results.getMetaData().getColumnCount(); i++)
         row.add(results.getObject(i));
      return row;
   }
   ...
}

Once I make these changes, the compiler leads me to similar required changes in PersisterTest (3 lines), Persister (2 lines), Persistable (1 line: change return type to Object), PersistableMetadata (1 line), CustomerAccess (3 lines), UserAccess, and SqlGenerator (several lines). It also leads to several changes stemming from StringUtil’s commaDelimit method (which can now take an array of Objects).

Digging around a bit more, I note a bigger problem. When creating the insert SQL string, the presumption is that all values are string literals. The result is that these values get wrapped with single quotes. That won’t work for numeric columns. The metadata for the domain object is going to have to include type information.

I add the following test to SqlGeneratorTest:

   public void testInsertNumericValue() {
      final Object[] values = { "1", 2 };
      final PersistableType[] types = {
         PersistableType.string, PersistableType.integer };
      assertEquals("insert into t (a,b) values ('1',2)",
         generator.createInsert(TABLE, COLUMNS, values, types));
   }

I’m still trying to take the path of least resistance. Adding a new argument to createInsert can be done without a lot of impact to other tests and code.

PersistableType is a new enum that will contain the mapping logic for each type.

package persistence;

import util.*;

public enum PersistableType {
   string {
      public String sqlValue(Object input) {
         if (input == null)
            return null;
         return StringUtil.wrap(input.toString(), '\'');
      }
   },
   integer {
      public String sqlValue(Object value) {
         if (value == null)
            return null;
         return value.toString();
      }
   };
   abstract String sqlValue(Object value);
}

Yes, it has tests:

IntegerTypeTest

package persistence;

import junit.framework.*;

public class IntegerTypeTest extends TestCase {
   private PersistableType type;

   protected void setUp() {
      type = PersistableType.integer;
   }

   public void testSqlValue() {
      assertEquals("1", type.sqlValue(1));
   }

   public void testNull() {
      assertNull(type.sqlValue(null));
   }
}

StringTypeTest

package persistence;

import junit.framework.*;

public class StringTypeTest extends TestCase {
   private PersistableType type;

   protected void setUp() {
      type = PersistableType.string;
   }

   public void testSqlValue() {
      assertEquals("'a'", type.sqlValue("a"));
   }

   public void testNullValue() {
      assertNull(type.sqlValue(null));
   }
}

If I were to modify methods in SqlGenerator by adding type information, it would break lots of other code. So instead I add overloaded methods.

SqlGenerator

package persistence;

import util.*;

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

   private String createValuesList(Object[] fields, final PersistableType[] types) {
      IndexTransformer transformer = new IndexTransformer() {
         public String transform(Object input, int i) {
            return types[i].sqlValue(input);
         }
      };
      return StringUtil.commaDelimit(fields, transformer);
   }
   ...
}

The ugly short-term solution I come up with was to introduce a new transformer, one that contains the index of each element passed to it. Once the transform method has that index, it can delegate to the sqlValue method of the appropriate type.

package util;

public interface IndexTransformer {
   String transform(Object object, int index);
}
package util;

public class StringUtil {
   ...
   public static String commaDelimit(Object[] objects, IndexTransformer transformer) {
      StringBuilder builder = new StringBuilder();
      for (int i = 0; i < objects.length; i++) {
         if (i > 0)
            builder.append(',');
         builder.append(transformer.transform(objects[i], i));
      }
      return builder.toString();
   }
   ...
}

Wow, this is a bit of work. The last part: the Persister class must obtain the appropriate type information. This requires a new method in the PersistableMetadata interface.

package persistence;

import java.util.*;

public interface PersistableMetadata {
   String getTable();
   String[] getColumns();
   String getKeyColumn();
   T create(List<Object> row);
   PersistableType[] getTypes();
}

The interface change impacts UserAccess, CustomerAccess, and PersisterTest. The method in CustomerAccess looks like:

   public PersistableType[] getTypes() {
      return new PersistableType[] {
         PersistableType.string, PersistableType.string, PersistableType.integer };
   }

Back to Customer, since fields are now carried about as untyped objects, they must be cast and returned appropriately.

public class Customer implements Persistable {
   ...
   private Map<String,Object> values = new HashMap<String,Object>();
   ...
   public String getId() {
      return (String)get(ID);
   }

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

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

Finally, success after about twenty minutes of fiddling around and changes to quite a few classes. Fortunately the changes were small and easy to find by using the compiler. But right now the solution is a bit of a mess. The thing that’s bugging me most is the disjoint metadata–all those separate arrays suck. Objects, anyone? Tomorrow will be some code cleanup.

Database TDD Part 17: Mocking Access

I have a failing test due to database dependencies:

package application;

import junit.framework.*;

public class ApplicationTest extends TestCase {
   public void testVerifyUser() {
      final String name = "name";
      final String password = "password";
      Application application = new Application();
      assertFalse(application.isRegistered(name, password));
      application.registerUser(name, password);
      assertTrue(application.isRegistered(name, password));
   }
}

I want to use an implementation of the UserAccess class that exists solely for controlling tests against Application. In the test, I introduce a setter method call to drop in a mock UserAccess implementation (to be defined):

   public void testVerifyUser() {
      final String name = "name";
      final String password = "password";
      Application application = new Application();
      application.setUserAccess(new MockUserAccess());
      assertFalse(application.isRegistered(name, password));
      application.registerUser(name, password);
      assertTrue(application.isRegistered(name, password));
   }

The Application class must change to accommodate this test intrusion:

package application;

import domain.*;

public class Application {
   private UserAccess userAccess = new UserAccess();

   public void registerUser(String name, String password) {
      User user = new User(name, password);
      userAccess.save(user);
   }

   public boolean isRegistered(String name, String password) {
      User user = userAccess.find(name);
      return user != null;
   }

   void setUserAccess(UserAccess userAccess) {
      this.userAccess = userAccess;
   }
}

I’d consider annotating the setUserAccess method with a comment saying it’s designed for testing use–but only if someone mentioned it.

The mock class is really dumb at this point. Its needs are minimal based on the existing content of ApplicationTests. The class will need to change as I add more comprehensive tests against Application.

package application;

import persistence.*;
import domain.*;

public class MockUserAccess extends UserAccess {
   private static User user;
   public User find(String key) {
      return MockUserAccess.user;
   }

   public void save(Persistable persistable) {
      MockUserAccess.user = (User)persistable;
   }
}

Subclass-based mocks can introduce some interesting issues. You do have to have a clear understanding of the behavior of the superclass, otherwise you run the risk of side effects whacking your tests. It’s generally safer to not go with this approach. Here, the benefit is that I don’t have to implement the unused PersistableMetadata methods.

Is this mocking tactic scaleable? What about when the Application class has to work with half a dozen persistence APIs? Do we want half a dozen setter methods used solely for the test? It’s too early for me to tell or even care.

The DataAccess subclasses, UserAccess and CustomerAccess, are still being tested using live JDBC calls. My take: that’s a good thing. It’s too easy for code to get out of sync with the database under test. If you have a few dozen DataAccess subclasses, perhaps they fall into the category of “integration’ tests. I’m not compelled to give them a Meaningful Name like that. But if they get in the way–if they start bogging things down in terms of execution time–I’ll foist them into a test suite that gets run just a bit less frequently. And, at that time, come up with a trustworthy mechanism to demonstrate that DataAccess subclasses and the Persister class are correctly coordinating with each other.

For now, I’m comfortable knowing that I could begin to build the rest of the application class in advance of any persistence details.

Database TDD Part 16: Driving Tests From the Application

Someone asked about the package organization of the project. In response, I moved the persistence-related classes into the persistence package. This required I believe one change to access level, and a number of import changes (which of course Eclipse will fix automatically).

I chose not to move the data access classes themselves–CustomerAccess, UserAccess, and associated test classes–to a different package yet. Keeping them with domain classes allows for some things to be stated at package level instead of public. Less work for now, in any case. I’m not worried about any level of effort in the future, since changes like these are easy and safe using Eclipse.

The new package organization:

  • domain.UserTest
  • domain.User
  • domain.UserAccessTest
  • domain.UserAccess
  • domain.CustomerTest
  • domain.Customer
  • domain.CustomerAccessTest
  • domain.CustomerAccess
  • persistence.DataAccess<T>
  • persistence.JdbcAccessTest
  • persistence.JdbcAccess
  • persistence.JdbcAccessExceptionsTest
  • persistence.JdbcException
  • persistence.Persistable
  • persistence.PersistableMetadata<T>
  • persistence.PersisterTest
  • persistence.Persister
  • persistence.SqlGeneratorTest
  • persistence.SqlGenerator

It’ll work for now.

The same someone was also concerned about the tests having to access the database so much. To start figuring out what’s going to work best, I want to write some application-level code.

package application;

import junit.framework.*;

public class ApplicationTest extends TestCase {
   public void testVerifyUser() {
      final String name = "name";
      final String password = "password";
      Application application = new Application();
      assertFalse(application.isRegistered(name, password));
      application.registerUser(name, password);
      assertTrue(application.isRegistered(name, password));
   }
}
package application;

import domain.*;

public class Application {
   public void registerUser(String name, String password) {
      User user = new User(name, password);
      new UserAccess().save(user);
   }

   public boolean isRegistered(String name, String password) {
      User user = new UserAccess().find(name);
      return user != null;
   }
}

OK, there’s a simple test, a failing one as expected. It’ll be my beacon for guidance over the next several minutes.

The test immediately introduces a problem: I don’t properly handle an empty result set in the JdbcAccess class. Driving a solution to this through tests, I first add a test for the Persister class that indicates what I want to happen when no rows are returned:

   public void testFindNotFound() {
      assertNull(persister.find(BAD_KEY));
   }

Other interesting elements in PersisterTest:

   private static final String BAD_KEY = "not found";

   protected void setUp() {
      access = new JdbcAccess() {
         ...
         public List<String> executeQuery(String sql) {
            lastSql = sql;
            if (sql.indexOf(BAD_KEY) > -1)
               return null;
            return EXPECTED_ROW;
         }
      };

The change to Persister is minor:

   public T find(String key) {
      String sql = new SqlGenerator().createFindByKey(metadata.getTable(), metadata.getColumns(),
            metadata.getKeyColumn(), key);
      List<String> row = access.executeQuery(sql);
      if (row == null)
         return null;
      return metadata.create(row);
   }

The Persister change in turn triggers the need for changes in JdbcAccess via JdbcAccessTest:

   public void testExecuteQueryNoResults() {
      access.execute(createTableSQL());
      assertNull(access.executeQuery("select x from " + TABLE + " where 1 = 0"));
   }

JdbcAccess:

   public List<String> executeQuery(String sql) {
      try {
         createStatement();

         ResultSet results = statement.executeQuery(sql);
         List<String> row = null;
         if (results.next())
            row = getRow(results);
         results.close();

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

Now I’m back to the original Application test, which is now failing. Why? Well, because it’s dealing with the database, and a previous run of the test added the user in question to the database tables.

There are a few options for a solution. The most obvious is to make sure that the database is cleaned out with each new unit test run. The better tactic is to start looking at inserting mocks. The dependence on the state of the database is one of many reasons we’ll want mocks for our unit tests. Other reasons are availability of the database, contention during concurrent test runs, and speed of the entire test run.

I’ll start dealing with these issues tomorrow.

Database TDD Part 15: Eliminating Duplication With Generics

The UserAccess and CustomerAccess classes each contain the method save and find. Here’s the code from one of them:

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

   public Customer find(String idKey) {
      return (Customer)new Persister(this).find(idKey);
   }

In save, the code looks exactly the same. The subtle difference is that the method contains a reference to this. I could easily push up thesave method to a common superclass, named perhaps DataAccess. That superclass would then need to implement the PersistableMetadata interface. There’s a minor nit: the name would bely the code contained within–the save method has nothing to do with metadata. I’m still searching for more appropriate names.

The find method is even more difficult, since it contains a cast to the specific type being retrieved. If I move that method directly to the superclass, its return type must change to Object. That would require clients to cast, something that’s unacceptable. I could also provide a subclass implementation with a variant return type, something else J2SE 5.0 supports, but then I’d have to provide a nuisance subclass method that delegated to the superclass. So much for completely eliminating duplication.

If you’re not working with J2SE 5.0, you’ll have to make the best of this. I’d probably not worry about it much. If you are using J2SE 5.0, you can use generics to eliminate the duplication. This requires a bunch of trivial changes to the code.

UserAccess

package domain;

import java.util.*;

public class UserAccess extends DataAccess<User> {
   public User create(List<String> row) {
      return new User(row.get(0), row.get(1));
   }

   public String getTable() {
      return "userdata";
   }

   public String[] getColumns() {
      return new String[] { User.NAME, User.PASSWORD };
   }

   public String getKeyColumn() {
      return User.NAME;
   }
}

The UserAccess class now extends the parameterized type DataAccess, which will contain the save and find methods. The extends in UserAccess binds DataAccess to the User type. Note that the create method directly returns the User type.

DataAcces

package domain;

abstract public class DataAccess<T> implements PersistableMetadata<T> {
   public void save(Persistable persistable) {
      new Persister<T>(this).save(persistable);
   }

   protected T find(String idKey) {
      return new Persister<T>(this).find(idKey);
   }
}

The DataAccess type is now parameterized. To avoid warnings, it’s also necessary to parameterize the Persister type, in addition to PersistableMetadata:

PersistableMetadata

package domain;

import java.util.*;

public interface PersistableMetadata<T> {
   String getTable();
   String[] getColumns();
   String getKeyColumn();
   T create(List<String> row);
}

Persister

package domain;

import java.util.*;

import persistence.*;

public class Persister<T> {
   private PersistableMetadata<T> metadata;
   private JdbcAccess access;

   public Persister(PersistableMetadata<T> metadata) {
      this(metadata, new JdbcAccess());
   }

   public Persister(PersistableMetadata<T> 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);
   }

   public T find(String key) {
      String sql = new SqlGenerator().createFindByKey(metadata.getTable(), metadata.getColumns(),
            metadata.getKeyColumn(), key);
      List<String> row = access.executeQuery(sql);
      return metadata.create(row);
   }
}

The PersisterTest class also must change to eliminate warnings. I just bound everything to Object for the simplest solution.

I like the result so far. UserAccess and CustomerAccess are about as duplication-free as they’re going to get. Here’s the CustomerAccess class:

CustomerAccess

package domain;

import java.util.*;

public class CustomerAccess extends DataAccess<Customer> {
   public String getTable() {
      return "cust";
   }

   public String[] getColumns() {
      return new String[] { Customer.ID, Customer.NAME };
   }

   public String getKeyColumn() {
      return Customer.ID;
   }

   public Customer create(List<String> row) {
      return new Customer(row.get(0), row.get(1));
   }
}

Database TDD Part 14: More Complex Mocking

Yesterday I extracted the common method save into the Persister class. The find can be dealt with almost as easily. The only trick is that there is a creation statement in it that builds an instance of the appropriate object (User or Customer). Before proceeding, then, step one is to extract this creation into a factory method. Step two is to update the PersistableMetadata interface so that it defines this creation method. (This suggests that the name for PersistableMetadata is no longer precise; we can deal with that later.)

After that, I move the findmethod to the Persister class. I then modify it to call back to the creation method defined on the access class. Here’s CustomerAccess:

package domain;

import java.util.*;

public class CustomerAccess implements PersistableMetadata {
   public void save(Persistable persistable) {
      new Persister(this).save(persistable);
   }

   public Customer find(String idKey) {
      return (Customer)new Persister(this).find(idKey);
   }

   public String getTable() {
      return "cust";
   }

   public String[] getColumns() {
      return new String[] { Customer.ID, Customer.NAME };
   }

   public String getKeyColumn() {
      return Customer.ID;
   }

   public Object create(List<String> row) {
      return new Customer(row.get(0), row.get(1));
   }
}

Since the columns list and table name are now only used in one place, I chose to inline them to the interface methods getColumns andgetKeyColumn. These methods are now constant methods.The updated PersistableMetadata interface:

package domain;

import java.util.*;

public interface PersistableMetadata {
   String getTable();
   String[] getColumns();
   String getKeyColumn();
   Object create(List<String> row);
}

The create method will have to return an Object, hence the cast in the CustomerAccess find method.

Persister:

package domain;

import java.util.*;

import persistence.*;

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

   public Persister(PersistableMetadata metadata) {
      this(metadata, new JdbcAccess());
   }

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

   public Object find(String key) {
      String sql =
       new SqlGenerator().createFindByKey(
        metadata.getTable(), metadata.getColumns(), metadata.getKeyColumn(), key);
      List<String> row = access.executeQuery(sql);
      return metadata.create(row);
   }
}

And finally, the missing test in PersistableTest:

package domain;

import java.util.*;

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

public class PersisterTest extends TestCase {
   private static final String TABLE = "x";
   private static final String[] COLUMNS = { "a", "b" };
   private static final List<String> EXPECTED_ROW = new ArrayList();
   private static final Object RETURN_OBJECT = "object";

   private JdbcAccess access;
   private String lastSql;
   private PersistableMetadata metadata;

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

         public List<String> executeQuery(String sql) {
            lastSql = sql;
            return EXPECTED_ROW;
         }
      };

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

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

         public String getKeyColumn() {
            return COLUMNS[0];
         }

         public Object create(List<String> row) {
            if (row == EXPECTED_ROW)
               return RETURN_OBJECT;
            return null;
         }
      };

   }

   public void testSave() {
      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;
         }
      };

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

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

   public void testFindBy() {
      final String key = "k";
      Persister persister = new Persister(metadata, access);
      assertEquals(RETURN_OBJECT, persister.find(key));
      assertEquals("select a,b from x where a='k'", lastSql);
   }
}

The method testFindBy was pretty easy to write, considering that I already had the Persistable, PersistableMetadata, and JdbcAccess test implementations/stubs. Here, I’ve added a stub for executeQuery and create. With refactoring to eliminate duplication in the test, the individual test methods are pretty succinct.

The Persister tests leave a slightly sour taste in my mouth, as do most tests where I introduce more complex mock interrelationships. Here, you have to dig around the test class a bit to understand the relationship between executeQuery, create, returnObject, andexpectedRow. I don’t have a problem with mocks per se, but they are best isolated to as few corners of your system as possible if they are at all tricky.

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.

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?

Atom