Database TDD Part 24: Refactoring After Breaks

Adding support for new where clause criteria is going to get very tedious very quickly. I want the client to be able to put this criteria together, but not by supplying SQL text strings. Instead, the client code should be able to supply a set of criteria objects in a parse tree form.

Starting with simple equality tests, I create an EqualsCriteria class:

EqualsCriteriaTest

package persistence;

import junit.framework.*;

public class EqualsCriteriaTest extends TestCase {
   private static final String FIELD_NAME = "field";

   public void testString() {
      Column column = new StringColumn(FIELD_NAME);
      Criteria criteria = new EqualsCriteria(column, "value");
      assertEquals("field='value'", criteria.sqlString());
   }

   public void testDifferentType() {
      Column column = new IntegerColumn(FIELD_NAME);
      Criteria criteria = new EqualsCriteria(column, 1);
      assertEquals("field=1", criteria.sqlString());
   }
}

I suppose I could get rid of the duplication in the assertions. Sometimes I think it’s easier to read the tests when the expected value is clearly spelled out in a complete, hardcoded string. But some other times, I think the J2SE 5.0 formatting capability allows for the assertion to be almost as succinct, while at the same time eliminating the duplication:

EqualsCriteriaTest, refactored

package persistence;

import junit.framework.*;

public class EqualsCriteriaTest extends TestCase {
   private static final String FIELD_NAME = "field";
   private Column column;

   public void testString() {
      column = new StringColumn(FIELD_NAME);
      final String value = "value";
      Criteria criteria = new EqualsCriteria(column, value);
      assertEquals(String.format("%s='%s'", FIELD_NAME, value), criteria.sqlString());
   }

   public void testDifferentType() {
      column = new IntegerColumn(FIELD_NAME);
      final int value = 1;
      Criteria criteria = new EqualsCriteria(column, value);
      assertEquals(String.format("%s=%d", FIELD_NAME, value), criteria.sqlString());
   }
}

The production class is pretty small and simple:

EqualsCriteria

package persistence;

public class EqualsCriteria implements Criteria {
   private Column column;
   private Object value;

   public EqualsCriteria(Column column, Object value) {
      this.column = column;
      this.value = value;
   }

   public String sqlString() {
      return String.format("%s=%s", column.getName(), column.sqlValue(value));
   }
}

The idea is that there will be multiple Criteria implementations, each with the ability to return an SQL string representation.

Criteria

package persistence;

public interface Criteria {
   String sqlString();
}

Putting together various other Criteria types is a quick, simple exercise.

LikeCriteriaTest

package persistence;

import junit.framework.*;

public class LikeCriteriaTest extends TestCase {
   private static final String FIELD_NAME = "field";

   public void testString() {
      Column column = new StringColumn(FIELD_NAME);
      final String pattern = "v%";
      Criteria criteria = new LikeCriteria(column, pattern);
      assertEquals(String.format("%s like '%s'", FIELD_NAME, pattern), criteria
            .sqlString());
   }

   // no other types are valid
}

LikeCriteria

package persistence;

public class LikeCriteria implements Criteria {
   private Column column;
   private String pattern;

   public LikeCriteria(Column column, String pattern) {
      this.column = column;
      this.pattern = pattern;
   }

   public String sqlString() {
      return String.format("%s like %s", column.getName(), column.sqlValue(pattern));
   }

}

There does seem to be some duplication creeping up between the various criteria types. But it’s not obvious to me yet how I’d eliminate it to any real benefit.

The fun part, and where this really starts paying off, is in adding support for logical operators. An “and” criteria introduces the recursive tree descent used to generate the entire where clause. It simply combines two other criteria, asking each for its SQL representation.

AndCriteriaTest

package persistence;

import junit.framework.*;

public class AndCriteriaTest extends TestCase {
   public void testString() {
      Criteria criteria1 = new LikeCriteria(new StringColumn("a"), "p%");
      Criteria criteria2 = new EqualsCriteria(new StringColumn("b"), "v");
      Criteria criteria = new AndCriteria(criteria1, criteria2);
      assertEquals(String.format("(%s) and (%s)", criteria1.sqlString(),
            criteria2.sqlString()), criteria.sqlString());
   }
}

AndCriteria

package persistence;

public class AndCriteria implements Criteria {
   private Criteria left;
   private Criteria right;

   public AndCriteria(Criteria left, Criteria right) {
      this.left = left;
      this.right = right;
   }

   public String sqlString() {
      return String
            .format("(%s) and (%s)", left.sqlString(), right.sqlString());
   }

}

Hmm, I just spotted some minor duplication with respect to the hardcoded SQL keywords I’ve been introducing (like, and). The code you download may have this fixed.

OrCriteriaTest

package persistence;

import junit.framework.*;

public class OrCriteriaTest extends TestCase {
   public void testString() {
      Criteria criteria1 = new LikeCriteria(new StringColumn("f1"), "v%");
      Criteria criteria2 = new EqualsCriteria(new StringColumn("f2"), 'x');
      Criteria criteria = new OrCriteria(criteria1, criteria2);
      assertEquals(String.format("(%s) or (%s)", criteria1.sqlString(),
            criteria2.sqlString()), criteria.sqlString());
   }
}

Looking at the implementation and test for OrCriteria, I do recognize a now significant amount of duplication.

OrCriteria

package persistence;

public class OrCriteria implements Criteria {
   private Criteria left;
   private Criteria right;

   public OrCriteria(Criteria left, Criteria right) {
      this.left = left;
      this.right = right;
   }

   public String sqlString() {
      return String
            .format("(%s) or (%s)", left.sqlString(), right.sqlString());
   }
}

Sometimes as I’m coding, I don’t really notice the duplication, or am too lazy to care about it (this is why I need a pair). I find that it’s a good idea to revisit the code a little later in my programming session, maybe when I come back after a 15-minute break. In fact, sometimes it’s hard to come up to speed after I’ve been distracted by something else, maybe a discussion of my fantasy team. A quick refactoring exercise after a break is a great way to refamiliarize myself with what I was just working on.

To make all these criteria classes useful, I’m going to need to modify how SQL gets generated.

SqlGeneratorTest

...
public void testCriteria() {
   String value = "v";
   String pattern = "p%";
   Criteria criteria = new AndCriteria(new EqualsCriteria(COLUMNS[0], value), 
      new LikeCriteria(COLUMNS[1], pattern));
   String sql = generator.createSelect(TABLE, COLUMNS, criteria);
   assertEquals("select a,b from t where (a='v') and (b like 'p%')", sql);
}
...

SqlGenerator

public String createSelect(String tableName, Column[] columns, Criteria criteria) {
   return String.format("%s where %s", createSelectAll(tableName, columns),
         criteria.sqlString());
}

It’s interesting how my methods seem to be getting shorter and shorter. This is a direct result of eliminating duplication all along: it tends to create more abstract, reusable methods, which allows me to more readily take advantage of them.

Backing out to the application, here’s some more code that readies the client to be able to use the Criteria classes.

PersisterTest

public void testFindWithCriteria() {
   Criteria criteria = new EqualsCriteria(COLUMNS[0], ROW1_VALUE1);
   assertQueryResults(persister.find(criteria));
   assertLastSql(String.format("select %s,%s from %s where %s='%s'",
         COLUMN1, COLUMN2, TABLE, COLUMN1, ROW1_VALUE1));
}

Persister

public List find(Criteria criteria) {
   return executeQuery(new SqlGenerator().createSelect(metadata.getTable(),
         metadata.getColumns(), criteria));
}

When I finally got to the existing, sample Application code, I found some new challenges that I’ll discuss tomorrow.

With respect to this blog, I wrote the code earlier in the day (about 30 minutes worth of coding), then came back and wrote the text around it. And as you can see, I wasn’t very happy with the code when I tried to describe it. Rather than rework the code, I figured maybe it was educational to show the first cut, and then, following, the refactored results.

CriteriaTestConstants

package persistence;

public class CriteriaTestConstants {
   static final String FIELD_NAME = "field";
   static final String STRING_VALUE = "value";
   static Column STRING_COLUMN = new StringColumn(FIELD_NAME);

   static final int INT_VALUE = 1;
   static Column INT_COLUMN = new IntegerColumn(FIELD_NAME);

   static final String PATTERN = "v%";
}

I don’t really care for “constants” classes, but for isolated test purposes I’m ok with it.

EqualsCriteriaTest

package persistence;

import junit.framework.*;
import static persistence.CriteriaTestConstants.*;

public class EqualsCriteriaTest extends TestCase {
   static final Criteria STRING_CRITERIA = new EqualsCriteria(STRING_COLUMN,
         STRING_VALUE);
   static final Criteria INT_CRITERIA = new EqualsCriteria(INT_COLUMN,
         INT_VALUE);

   public void testString() {
      assertEquals(String.format("%s='%s'", FIELD_NAME, STRING_VALUE),
            STRING_CRITERIA.sqlString());
   }

   public void testDifferentType() {
      assertEquals(String.format("%s=%d", FIELD_NAME, INT_VALUE), INT_CRITERIA
            .sqlString());
   }
}

EqualsCriteria

package persistence;

public class EqualsCriteria extends BinaryOperator implements Criteria {
   public EqualsCriteria(Column column, Object value) {
      super(column, value);
   }

   public String sqlString() {
      return super.binaryClause("=");
   }
}

This common superclass might end up with a better name later:

BinaryOperator

package persistence;

public class BinaryOperator {
   private Column column;
   private Object value;

   public BinaryOperator(Column column, Object value) {
      this.column = column;
      this.value = value;
   }

   public String binaryClause(String operator) {
      return String.format("%s%s%s", column.getName(), operator, column
            .sqlValue(value));
   }
}

LikeCriteriaTest

package persistence;

import junit.framework.*;
import static persistence.CriteriaTestConstants.*;

public class LikeCriteriaTest extends TestCase {
   static final Criteria CRITERIA = new LikeCriteria(STRING_COLUMN, PATTERN);

   public void testString() {
      assertEquals(String.format("%s%s'%s'", FIELD_NAME, LikeCriteria.LIKE,
            PATTERN), CRITERIA.sqlString());
   }

   // no other types are valid
}

LikeCriteria

package persistence;

public class LikeCriteria extends BinaryOperator implements Criteria {
   static final String LIKE = " like ";

   public LikeCriteria(Column column, String pattern) {
      super(column, pattern);
   }

   public String sqlString() {
      return super.binaryClause(LIKE);
   }
}

AndCriteriaTest

package persistence;

import junit.framework.*;

public class AndCriteriaTest extends TestCase {
   public void testString() {
      Criteria criteria1 = LikeCriteriaTest.CRITERIA;
      Criteria criteria2 = EqualsCriteriaTest.STRING_CRITERIA;
      Criteria criteria = new AndCriteria(criteria1, criteria2);
      assertEquals(String.format("(%s) %s (%s)", criteria1.sqlString(), AndCriteria.AND,
            criteria2.sqlString()), criteria.sqlString());
   }
}

AndCriteria

package persistence;

public class AndCriteria extends LogicalCriteria implements Criteria {
   public static final String AND = "and";

   public AndCriteria(Criteria left, Criteria right) {
      super(left, right);
   }

   public String sqlString() {
      return binarySqlString(AND);
   }
}

LogicalCriteria

package persistence;

public class LogicalCriteria {
   protected Criteria left;
   protected Criteria right;

   public LogicalCriteria(Criteria left, Criteria right) {
      this.left = left;
      this.right = right;
   }

   protected String binarySqlString(String operator) {
      return String.format("(%s) %s (%s)", left.sqlString(), operator, right
            .sqlString());
   }
}

OrCriteriaTest

package persistence;

import junit.framework.*;

public class OrCriteriaTest extends TestCase {
   public void testString() {
      Criteria criteria1 = EqualsCriteriaTest.STRING_CRITERIA;
      Criteria criteria2 = LikeCriteriaTest.CRITERIA;
      Criteria criteria = new OrCriteria(criteria1, criteria2);
      assertEquals(String.format("(%s) %s (%s)", criteria1.sqlString(), OrCriteria.OR,
            criteria2.sqlString()), criteria.sqlString());
   }
}

OrCriteria

package persistence;

public class OrCriteria extends LogicalCriteria implements Criteria {
   public static final String OR = "or";

   public OrCriteria(Criteria left, Criteria right) {
      super(left, right);
   }

   public String sqlString() {
      return binarySqlString(OR);
   }
}

There are still some areas of duplication. For now, further refactoring would seem to hold diminishing returns.

Database TDD Part 23: Select Criteria

The pseudo-application I’m building is going to have varied query needs, involving multiple conditionals, matching strings, relational operators, and so on. I want the ability to be more flexible with my where clauses. So far, the existing database layer supports only finding a row by key. I’m going to add the ability to support a second type of criteria, a matching operation against a single column.

Driving from the application down:

ApplicationTest

public void testBrowseCustomers() {
   final String id1 = "id1";
   final String name1 = "name1";

   application.setCustomerAccess(new MockCustomerAccess());

   Customer customer1 = new Customer(id1, name1);
   application.add(customer1);
   assertTrue(application.findMatchingCustomers("a%").isEmpty());
   List customers = application.findMatchingCustomers("n%");
   assertEquals(1, customers.size());
   Customer retrievedCustomer = customers.get(0);
   assertEquals(id1, retrievedCustomer.getId());
   assertEquals(name1, retrievedCustomer.getName());
}

The above test demonstrates the ability to search against the customer name using SQL wildcard characters (%).

As I often do, I’ll dig my way down through code, laying in the interface, until I find the point at which I can insert some real code, with tests of course. Then I’ll start working my way back up to the application level. The application code I want to get working:

ApplicationTest

public class Application {
   ...
   private CustomerAccess customerAccess = new CustomerAccess();
   ...
   void setCustomerAccess(CustomerAccess customerAccess) {
      this.customerAccess = customerAccess;
   }
   ...
   public void add(Customer customer) {
      customerAccess.save(customer);
   }

   public List findMatchingCustomers(String namePattern) {
      return customerAccess.findMatches(Customer.NAME, namePattern);
   }
}

I had to make the Customer constant NAME public. I don’t think that’s a bad thing, but for now I chose to make only that constant public and not any other constants.

I built the MockCustomerAccess class to contain a bit of logic. It converts the SQL pattern matching string into a regex string (presuming that only the wildcard character is specified). This code starts treading some dangerous waters. Mock code should generally be simple and straightforward, meeting only the purposes of the tests. If you find yourself maintaining mock code more than you find yourself maintaining production code, you’ve uncovered a nasty smell. I’ll monitor the need to revisit this code in the future.

MockCustomerAccess

package application;

import java.util.*;

import persistence.*;
import domain.*;

class MockCustomerAccess extends CustomerAccess {
   private static List customers = new ArrayList();

   public List findMatches(String columnName, String pattern) {
      String regexPattern = pattern.replaceAll("%", ".*");
      List results = new ArrayList();
      for (Customer customer: customers)
         if (((String)customer.get(columnName)).matches(regexPattern))
            results.add(customer);
      return results;
   }

   public void save(Persistable persistable) {
      customers.add((Customer)persistable);
   }
}

The findMatches method ends up being implemented on the DataAccess superclass, since it’s just as generic as anything else. As soon as I started coding it I recognized that it would have to translate a column name to a Column object. This makes for simpler client code, and also encapsulates client code from any changes to the Column interface.

DataAccess

public List findMatches(String columnName, String namePattern) {
   Column column = getColumn(columnName);
   return new Persister(this).findMatches(column, namePattern);
}

public Column getColumn(String name) {
   for (Column column: getColumns())
      if (column.getName().equals(name))
         return column;
   return null;
}

Of course, I built the getColumn method test-first. Here’s the new test class for DataAccess:

DataAccessTest

package persistence;

import java.util.*;
import junit.framework.*;

public class DataAccessTest extends TestCase {
   private static final String NAME1 = "col1";
   private static final String NAME2 = "col2";
   private static final Column COLUMN1 = new StringColumn(NAME1);
   private static final Column COLUMN2 = new StringColumn(NAME2);

   public void testGetColumn() {
      DataAccess access = new DataAccess() {
         public String getTable() {
            return null;
         }

         public String getKeyColumn() {
            return null;
         }

         public Object create(Map row) {
            return null;
         }

         public Column[] getColumns() {
            return new Column[] { COLUMN1, COLUMN2 };
         }
      };

      assertEquals(COLUMN1, access.getColumn(NAME1));
      assertEquals(COLUMN2, access.getColumn(NAME2));
   }
}

Moving down the chain, PersisterTest is always the big bit of pain in writing tests, since it’s where the JDBC mocking comes into play. The listing here shows a bit of refactoring that I did in order to make the tests short and simple.

PersisterTest

package persistence;

import java.util.*;

import junit.framework.*;

public class PersisterTest extends TestCase {
   private static final String TABLE = "x";
   private static final Object RETURN_OBJECT1 = "object";
   private static final Object RETURN_OBJECT2 = "object2";
   private static final String BAD_KEY = "not found";
   private static final String COLUMN1 = "a";
   private static final String COLUMN2 = "b";
   private static final String ROW1_VALUE1 = "a1";
   private static final String ROW1_VALUE1_PATTERN = "a%";
   private static final String ROW1_VALUE2 = "a2";
   private static final String ROW2_VALUE1 = "b1";
   private static final String ROW2_VALUE2 = "b2";
   private static final Column[] COLUMNS = { new StringColumn(COLUMN1),
         new StringColumn(COLUMN2) };

   private JdbcAccess access;
   private String lastSql;
   private PersistableMetadata<object width="300" height="150"> metadata;
   private Persister<object> persister;
   private Persistable persistable;

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

         public Map&lt;String, Object&gt; executeQueryExpectingOneRow(String sql,
               Column[] columns) {
            lastSql = sql;
            if (sql.indexOf(BAD_KEY) &gt; -1)
               return null;
            return createRow1();
         }

         public List&lt;Map&lt;String, Object&gt;&gt; executeQuery(String sql,
               Column[] columns) {
            lastSql = sql;
            return createTwoRows();
         }
      };

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

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

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

         public Object create(Map&lt;String, Object&gt; row) {
            if (row.get(COLUMN1).equals(ROW1_VALUE1)
                  &amp;&amp; row.get(COLUMN2).equals(ROW1_VALUE2))
               return RETURN_OBJECT1;
            if (row.get(COLUMN1).equals(ROW2_VALUE1)
                  &amp;&amp; row.get(COLUMN2).equals(ROW2_VALUE2))
               return RETURN_OBJECT2;
            return null;
         }
      };

      persistable = new Persistable() {
         public Object get(String key) {
            if (key.equals(COLUMN1))
               return ROW1_VALUE1;
            if (key.equals(COLUMN2))
               return ROW1_VALUE2;
            return null;
         }
      };

      persister = new Persister<object>(metadata, access);
   }

   protected Map&lt;String, Object&gt; createRow1() {
      return createRow(ROW1_VALUE1, ROW1_VALUE2);
   }

   protected Map&lt;String, Object&gt; createRow2() {
      return createRow(ROW2_VALUE1, ROW2_VALUE2);
   }

   private List&lt;Map&lt;String, Object&gt;&gt; createTwoRows() {
      List&lt;Map&lt;String, Object&gt;&gt; results = new ArrayList&lt;Map&lt;String, Object&gt;&gt;();
      results.add(createRow1());
      results.add(createRow2());
      return results;
   }

   protected Map&lt;String, Object&gt; createRow(String value1, String value2) {
      Map&lt;String, Object&gt; row = new HashMap&lt;String, Object&gt;();
      row.put(COLUMN1, value1);
      row.put(COLUMN2, value2);
      return row;
   }

   public void testSave() {
      persister.save(persistable);
      assertLastSql(String.format(
            "insert into %s (%s,%s) values ('%s','%s')", TABLE, COLUMN1,
            COLUMN2, ROW1_VALUE1, ROW1_VALUE2));
   }

   public void testFindBy() {
      final String key = ROW1_VALUE1;
      assertEquals(RETURN_OBJECT1, persister.find(key));
      assertLastSql(String.format("select %s,%s from %s where %s='%s'",
            COLUMN1, COLUMN2, TABLE, COLUMN1, key));
   }

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

   public void testGetAll() {
      assertQueryResults(persister.getAll());
      assertEquals(String.format("select %s,%s from %s", COLUMN1,
            COLUMN2, TABLE), lastSql);
   }

   public void testFindMatches() {
      assertQueryResults(persister.findMatches(COLUMNS[0], ROW1_VALUE1_PATTERN));
      assertLastSql(String.format("select %s,%s from %s where %s like '%s'",
            COLUMN1, COLUMN2, TABLE, COLUMN1, ROW1_VALUE1_PATTERN));
   }

   private void assertLastSql(String sql) {
      assertEquals(sql, lastSql);
   }

   private void assertQueryResults(List<object> results) {
      assertEquals(2, results.size());
      assertTrue(results.contains(RETURN_OBJECT1));
      assertTrue(results.contains(RETURN_OBJECT2));
   }
}


The new test method is <code>testFindMatches</code>. It executes the <code>findMatches</code> method against the Persister class. The test then verifies two things: that the correct SQL was built and passed to the JDBC <code>executeQuery</code> method, and that the (hardcoded) results of the query were gathered correctly (<code>assertQueryResults</code>).

Within Persister, the new <code>findMatches</code> method has a lot in common with <code>getAll</code>. Both need to translate a set of results into domain objects. The only distinction is in the where clause of the SQL select statement.
<h4>Persister</h4>
<pre>public List<t> getAll() {
   String sql = new SqlGenerator().createSelectAll(metadata.getTable(),
         metadata.getColumns());
   return executeQuery(sql);
}

public List<t> findMatches(Column column, String pattern) {
   String sql = new SqlGenerator().createSelect(metadata.getTable(),
         metadata.getColumns(), column, pattern);
   return executeQuery(sql);
}

private List<t> executeQuery(String sql) {
   List&lt;Map&lt;String, Object&gt;&gt; rows = access.executeQuery(sql, metadata
         .getColumns());
   List<t> results = new ArrayList<t>(rows.size());
   for (Map&lt;String, Object&gt; row : rows)
      results.add(metadata.create(row));
   return results;
}
</t></t></t></t></t>

Here are SqlGeneratorTest and SqlGenerator. I refactored SqlGenerator after adding createSelect.

SqlGeneratorTest

public class SqlGeneratorTest extends TestCase {
   private static Column[] COLUMNS = { new StringColumn("a"),
         new StringColumn("b") };
   ...
   public void testFindMatches() {
      final String pattern = "v%";
      final Column column = COLUMNS[1];
      assertEquals("select a,b from t where b like 'v%'", generator
            .createSelect(TABLE, COLUMNS, column, pattern));
   }
}

SqlGenerator

public class SqlGenerator {
   ...
   public String createSelectAll(String table, Column[] columns) {
      return String.format("select %s from %s", createColumnList(columns),
            table);
   }

   public String createFindByKey(String tableName, Column[] columns,
         String keyColumn, String keyValue) {
      final String criteria = String.format("%s='%s'", keyColumn, keyValue);
      return createSelectWithCriteria(tableName, columns, criteria);
   }

   public String createSelect(String tableName, Column[] columns,
         Column column, String pattern) {
      final String criteria = String.format("%s like '%s'", column.getName(),
            pattern);
      return createSelectWithCriteria(tableName, columns, criteria);
   }

   private String createSelectWithCriteria(String tableName, Column[] columns,
         String criteria) {
      String whereClause = String.format("where %s", criteria);
      return String.format("%s %s", createSelectAll(tableName, columns),
            whereClause);
   }
}

Where clause criteria needs are going to change. As mentioned earlier, I’ll need multiple conditionals and so on. Right now I’m ok with what I have, but I want to soon shift the burden of constructing a where clause to the client. Otherwise the SqlGenerator class and other code will need to change infinitely into the future, with almost each new query conditional that a client requires. Fixing that is a future installment.

Worthless Comments

I’ll get back to the database layer next week. Yesterday I received my copy of a well-known Java magazine in the mail, and it prompted me to write this entry. Usually the things that prompt me to write are not good, and this was no exception.

I was provoked by an article that presents a distillation of design patterns in Java. I wouldn’t know how well the article describes the patterns–I couldn’t get past the code examples.

Here’s a few example pieces of code from the article. Fair use allows me to present this snippet here:

public class Memento {
/** stores the State instance of the Memento */
   private State fState;

   ...

/**
 * This method appends a new item to the collection of items
 */
public void append(Object item) {
...

And on, and on, and on. Ad nauseum. Javadoc comments like this often exist because someone mandates Javadoc on all methods. Never mind that for the append method, the Javadoc tool will supply perfectly sufficient information based off the method signature. People think that they’re being helpful by providing such comments. In fact, they are causing me to waste time. I can’t see the code for the clutter of comments. With well-written, well-composed methods, methods fit in one small chunk on the screen and use intention-revealing names. I can understand such code far more rapidly than equivalent code with useless comments.

I’ve engaged in many discussions in various forums (mostly Yahoo groups) about commenting. There are always people who insist on a larger number of comments than are necessary in a well-written application.

The problem I have is that the people defending excessive comments rarely concede that there are a large number of worthless comments that you should just eliminate. My view is that the majority of comments can be rewritten as better code. Certainly, there will still be the need for comments in your code. I don’t deny that.

On one of the lists, maybe the TDD list or the Scrum list, a poster suggested a good example that probably warranted a comment. OK, great. That’s one example successfully defended, leaving 99.9999% of all comments unsuccessfully defended.

It’s the same argument as with short methods: people will insist that there are legitimate reasons to code 200+ line methods. McConnell does so in Code Complete, which I helped review. Fine, there are (a very few) legitimate reasons to add comments and there are (even fewer) legitimate reasons to concede excessive length in some methods.

But why the argument? Instead of wasting breath defending the rare exception, the larger value is in learning what to do about everything else. Look at every method with a critical eye, and try to find a way to make it shorter or clearer through elimination of comments. That’s all I ask.

Comments:

I’m sure someone will read this and go away thinking it says to never add comments.

Jeff said “don’t write comments.”

Database TDD Part 22: Multiple Rows

Lots of code today. The application needs to be able to obtain a list of all the user names that are registered in the system. Starting at the application layer:

ApplicationTest

package application;

import java.util.*;

import junit.framework.*;

public class ApplicationTest extends TestCase {
   final String name1 = "name1";
   final String password1 = "password1";
   final String name2 = "name2";
   final String password2 = "password2";

   private Application application;

   protected void setUp() {
      application = new Application();
      application.setUserAccess(new MockUserAccess());
   }

   public void testVerifyUser() {
      assertFalse(application.isRegistered(name1, password1));
      application.registerUser(name1, password1);
      assertTrue(application.isRegistered(name1, password1));
   }

   public void testRegisteredUsers() {
      application.registerUser(name1, password1);
      application.registerUser(name2, password2);
      Set<String> users = application.getRegisteredUsers();
      assertEquals(2, users.size());
      assertTrue(users.contains(name1));
      assertTrue(users.contains(name2));
   }
}

Application

package application;

import java.util.*;

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

   public Set<String> getRegisteredUsers() {
      Set<String> results = new HashSet<String>();
      for (User user: userAccess.getAll())
         results.add(user.getName());
      return results;
   }
}

To build this solution, I drove all the way down from the application layer, programming by intent. Here in the Application class, the job was to flesh out what the getRegisteredUsers method might look like. I declared the need for a getAll method defined in the data access class.

Getting the code to work meant updating the mock class.

MockUserAccess

...
public class MockUserAccess extends UserAccess {
   private static User user;
   private static List<User> users = new ArrayList<User>();

   public User find(String key) {
      return MockUserAccess.user;
   }

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

   public List<User> getAll() {
      return users;
   }
}

Note that there are two kinds of things going on here. I’m not worrying about that yet.

DataAccess

   public List<T> getAll() {
      return new Persister<T>(this).getAll();
   }

I just realized that this method won’t be getting tested as long as I’m using the mock class in ApplicationTest. Looks like a future exercise needs to be an acceptance level test. Right now I’m only concerned about building the units. Acceptance tests are a good end-of-day exercise. I figure so far, from blog entry #1 to #22, I’m probably up to 2:30 in the afternoon, were I to have been working on this codebase nonstop.

PersisterTest

package persistence;

import java.util.*;

import junit.framework.*;

public class PersisterTest extends TestCase {
   private static final String TABLE = "x";
   private static final Object RETURN_OBJECT1 = "object";
   private static final Object RETURN_OBJECT2 = "object2";
   private static final String BAD_KEY = "not found";
   private static final String COLUMN1 = "a";
   private static final String COLUMN2 = "b";
   private static final String ROW1_VALUE1 = "a1";
   private static final String ROW1_VALUE2 = "a2";
   private static final String ROW2_VALUE1 = "b1";
   private static final String ROW2_VALUE2 = "b2";
   private static final Column[] COLUMNS = { new StringColumn(COLUMN1),
         new StringColumn(COLUMN2) };

   private JdbcAccess access;
   private String lastSql;
   private PersistableMetadata<Object> metadata;
   private Persister<Object> persister;
   private Persistable persistable;

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

         public Map<String, Object> executeQueryExpectingOneRow(String sql,
               Column[] columns) {
            lastSql = sql;
            if (sql.indexOf(BAD_KEY) > -1)
               return null;
            return createRow1();
         }

         public List<Map<String, Object>> executeQuery(String sql,
               Column[] columns) {
            lastSql = sql;

            List<Map<String, Object>> results = new ArrayList<Map<String, Object>>();
            results.add(createRow1());
            results.add(createRow2());
            return results;
         }
      };

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

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

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

         public Object create(Map<String, Object> row) {
            if (row.get(COLUMN1).equals(ROW1_VALUE1)
                  && row.get(COLUMN2).equals(ROW1_VALUE2))
               return RETURN_OBJECT1;
            if (row.get(COLUMN1).equals(ROW2_VALUE1)
                  && row.get(COLUMN2).equals(ROW2_VALUE2))
               return RETURN_OBJECT2;
            return null;
         }
      };

      persistable = new Persistable() {
         public Object get(String key) {
            if (key.equals(COLUMN1))
               return ROW1_VALUE1;
            if (key.equals(COLUMN2))
               return ROW1_VALUE2;
            return null;
         }
      };

      persister = new Persister<Object>(metadata, access);
   }

   protected Map<String, Object> createRow1() {
      return createRow(ROW1_VALUE1, ROW1_VALUE2);
   }

   protected Map<String, Object> createRow2() {
      return createRow(ROW2_VALUE1, ROW2_VALUE2);
   }

   protected Map<String, Object> createRow(String value1, String value2) {
      Map<String, Object> row = new HashMap<String, Object>();
      row.put(COLUMN1, value1);
      row.put(COLUMN2, value2);
      return row;
   }

   public void testSave() {
      persister.save(persistable);

      String expectedSql = String.format(
            "insert into %s (%s,%s) values ('%s','%s')", TABLE, COLUMN1,
            COLUMN2, ROW1_VALUE1, ROW1_VALUE2);
      assertEquals(expectedSql, lastSql);
   }

   public void testFindBy() {
      final String key = ROW1_VALUE1;
      assertEquals(RETURN_OBJECT1, persister.find(key));
      String expectedSql = String.format("select %s,%s from %s where %s='%s'",
            COLUMN1, COLUMN2, TABLE, COLUMN1, key);
      assertEquals(expectedSql, lastSql);
   }

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

   public void testGetAll() {
      persister.save(persistable);

      Persistable persistable2 = new Persistable() {
         public Object get(String key) {
            if (key.equals(COLUMN1))
               return ROW2_VALUE1;
            if (key.equals(COLUMN2))
               return ROW2_VALUE2;
            return null;
         }
      };
      persister.save(persistable2);
      List<Object> results = persister.getAll();

      String expectedSql = String.format("select %s,%s from %s", COLUMN1,
            COLUMN2, TABLE);
      assertEquals(expectedSql, lastSql);

      assertEquals(2, results.size());
      assertTrue(results.contains(RETURN_OBJECT1));
      assertTrue(results.contains(RETURN_OBJECT2));
   }
}

This class got a bit of refactoring. The concept for testing “get all” is pretty much the same as testing find-by, except that there are multiple rows that need to be verified. I’m still not enamored of the complexity in setting up the tests in this class.

Persister

...
public class Persister<T> {
   ...
   public List<T> getAll() {
      String sql = new SqlGenerator().createSelectAll(metadata.getTable(), metadata.getColumns());
      List<Map<String, Object>> rows = access.executeQuery(sql, metadata.getColumns());
      List<T> results = new ArrayList<T>(rows.size());
      for (Map<String, Object> row: rows)
         results.add(metadata.create(row));
      return results;
   }
}

The new method in Persister is much like the find method, except that once again it’s dealing with multiple rows. Here I had to comment code in getAlluntil I was able to get to a point where it would compile. That meant a diversion to get the new SqlGenerator and JdbcAccess methods in place. That’s my process: write a test, attempt to implement method by intent, comment out non-compiling code, write a test at the lower level as necessary, uncomment out compile code and retest. There are other ways of doing this, of course.

SqlGenerator code was pretty straightforward:

SqlGeneratorTest

   public void testGetAll() {
      assertEquals("select a,b from t", generator.createSelectAll(TABLE, COLUMNS));
   }

SqlGenerator

   public String createSelectAll(String table, Column[] columns) {
      return String.format("select %s from %s", createColumnList(columns),
            table);
   }

Hmm, some minor duplication between that and createFindByKey that I had forgotten about. Let me go back and fix that…

   public String createFindByKey(String tableName, Column[] columns,
         String keyColumn, String keyValue) {
      return String.format("%s where %s='%s'",
         createSelectAll(tableName, columns), keyColumn, keyValue);
   }

You’ll note that in Persister’s getAll method, the return value from executeQuery is a List of rows (maps), instead of a single map that represents a row. In order to build this code incrementally, I first named the new form of the executeQuery method executeQueryMultipleResults. Once I got everything working, I went back and flipped the method names around (so that the abnormal case–executing a query and expecting one row–was the more explicitly named method).

Here’s JdbcAccessTest:

package persistence;

import java.util.*;

import junit.framework.*;

public class JdbcAccessTest extends TestCase {
   ...
   private static final String VALUE1 = "a";
   private static final String VALUE2 = "b";
   ...
   public void testExecuteQuery() {
      insertRow();
      assertRow(access.executeQueryExpectingOneRow(createSelectSql(), COLUMNS));
   }
   ...
   public void testExecuteQueryNoResults() {
      assertNull(access.executeQueryExpectingOneRow(createSelectSql() + " where 1=0", COLUMNS));
   }

   public void testExecuteQueryMultipleResults() {
      insertRow(VALUE1);
      insertRow(VALUE2);

      List<Map<String, Object>> rows = access.executeQuery(createSelectSql(), COLUMNS);
      assertEquals(2, rows.size());
      assertContains(rows, VALUE1);
      assertContains(rows, VALUE2);
   }

   private void assertContains(List<Map<String, Object>> rows, String columnValue) {
      Map<String, Object> expectedRow = new HashMap<String, Object>();
      expectedRow.put(COLUMN_NAME, columnValue);
      assertTrue(rows.contains(expectedRow));
   }

   private void assertRow(Map<String, Object> row) {
      assertEquals(1, row.size());
      assertEquals(VALUE1, row.get(COLUMN_NAME));
   }

   private void insertRow() {
      insertRow(VALUE1);
   }

   private void insertRow(String value) {
      String sql = String.format("insert into %s values('%s')", TABLE, value);
      access.execute(sql);
   }
   ...
}

And finally, JdbcAccess, to which I added one comment regarding something that I might want to look at for performance considerations later.

JdbcAccess

...
public class JdbcAccess {
   ...
   public Map<String, Object> executeQuery(String sql) {
      return executeQueryExpectingOneRow(sql, null);
   }

   public Map<String, Object> executeQueryExpectingOneRow(String sql, Column[] columns) {
      List<Map<String, Object>> rows = executeQuery(sql, columns);
      if (rows.isEmpty())
         return null;
      return rows.get(0);
   }

   public List<Map<String, Object>> executeQuery(String sql, Column[] columns) {
      try {
         createStatement();

         ResultSet results = statement.executeQuery(sql);
         List<Map<String,Object>> rows = new ArrayList<Map<String,Object>>();

         if (columns == null) // what if results is empty, wasteful
            columns = createPseudoColumns(results.getMetaData());

         while (results.next()) {
            Map<String, Object> row = getRow(results, columns);
            rows.add(row);
         }
         results.close();

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

One thing I derived from today’s exercise: adding new functionality was a pretty straightforward exercise of digging down through the layers, with a bit of refactoring cleanup along the way. A lot of the code necessary was already in place. As I add more functionality, it creates new groundwork that makes addition of future functionality even easier.

Database TDD Part 21: Revisiting Performance

The casting in the domain classes is going to continue to be a major nuisance. The Customer class is already demonstrating a good amount of casting clutter for such a small class.

Customer

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

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

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

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

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

Casting is a form of duplication that I can get rid of in several ways. One solution would be to create a different collection for each stored type. Another solution would be to isolate it to a separate class, so none of the domain classes would need to cast.

Each domain class can store an instance of a FieldMap, which encapsulates all the nuisances of casting. It can also handle common update operations as they are coded, such as adding to an already-stored value (like the charge method does).

FieldMapTest

package domain;

import junit.framework.*;

public class FieldMapTest extends TestCase {
   private static final String KEY = "key";
   private FieldMap map;

   protected void setUp() {
      map = new FieldMap();
   }

   public void testGet() {
      Object object = new Object();
      map.put(KEY, object);
      assertSame(object, map.get(KEY));
   }

   public void testString() {
      final String value = "abc";
      map.put(KEY, value);
      assertEquals(value, map.getString(KEY));
   }

   public void testNullString() {
      map.put(KEY, null);
      assertNull(map.getString(KEY));
   }

   public void testStringNotPut() {
      assertNull(map.getString(KEY));
   }

   public void testInteger() {
      map.put(KEY, 1);
      assertEquals(1, map.getInt(KEY));
   }

   public void testIntegerNotPut() {
      assertEquals(0, map.getInt(KEY));
   }

   public void testAdd() {
      map.put(KEY, 1);
      map.add(KEY, 2);
      assertEquals(3, map.getInt(KEY));
   }
}

FieldMap

package domain;

import java.util.*;

public class FieldMap {
   private Map<String,Object> values = new HashMap<String,Object>();

   public void put(String key, Object value) {
      values.put(key, value);
   }

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

   public int getInt(String key) {
      Integer value = (Integer)values.get(key);
      if (value == null)
         return 0;
      return value.intValue();
   }

   public void add(String key, int amount) {
      int value = getInt(key);
      put(key, value + amount);
   }

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

}

Modifications to the domain classes get rid of much of the clutter.

Customer

package domain;

import persistence.*;

public class Customer implements Persistable {
   static final String ID = "id";
   static final String NAME = "name";
   static final String BALANCE = "balance";

   private FieldMap values = new FieldMap();

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

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

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

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

   public void charge(int amount) {
      values.add(BALANCE, amount);
   }

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

User

package domain;

import persistence.*;

public class User implements Persistable {
   static final String PASSWORD = "password";
   static final String NAME = "name";
   private FieldMap values = new FieldMap();

   public User(String name, String password) {
      values.put(NAME, name);
      values.put(PASSWORD, password);
   }

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

   public String getPassword() {
      return values.getString(PASSWORD);
   }

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

There’s still the common get method and declaration of the values FieldMap instance. The get method is expected by clients of the Persistable interface. I could conceivably foist those elements into a superclass, but I’m going to hold off a bit on that.

One concern that use of the FieldMap might bring up is its overhead in performance cost. With respect to numeric operations, the code now must wrap and unwrap int values in order to do any math with them. It’s pretty unlikely that anything here would truly present a problem–the real overhead in production will be the cost of persistence, not the cost of wrapping and unwrapping. But let’s say that it does become an issue.

As a pretty contrived example, suppose I’ve been told that the charge method must support 5,000 operations per second. Currently, it’s only supporting about 1,000 operations per second.

First step is, as always, find some way to measure the existing performance.

User

package domain;

import junit.framework.*;

public class CustomerPerformanceTest extends TestCase {
   public void testBalance() {
      Customer customer = new Customer("","");
      final int amount = 2;
      final int iterations = 1000;
      assertEquals(0, customer.getBalance());

      long start = System.nanoTime();
      for (int i = 0; i < iterations; i++)
         customer.charge(amount);
      assertEquals(amount * iterations, customer.getBalance());
      long stop = System.nanoTime();

      long microseconds = (stop - start) / 1000;
      assertTrue("microseconds: " + microseconds, microseconds < 200);
   }
}

Sure enough, this test fails fairly consistently (on my sluggish laptop). It takes about 800 microseconds to complete 1000 iterations.

The design is flexible enough to allow me to optimize things:

Customer

package domain;

import persistence.*;

public class Customer implements Persistable {
   static final String ID = "id";
   static final String NAME = "name";
   static final String BALANCE = "balance";
   // balance is cached to meet performance goals
   private int balance = 0;

   private FieldMap values = new FieldMap();

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

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

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

   public int getBalance() {
      return balance;
   }

   public void charge(int amount) {
      balance += amount;
   }

   public Object get(String key) {
      if (key == BALANCE)
         return new Integer(balance);
      return values.get(key);
   }
}

I added the balance field to allow for direct int manipulation. When the persistence framework needs to access the balance, code in the get method uses a guard clause to treat the balance as a specially cached value.

Comments are important here. Optimization is usually a curious looking thing in the code. Developers can innocently refactor performance optimization away if it’s not clear why it exists. Stating the performance goals in the form of tests can go a long way toward diminishing the verbosity of such comments. Short-term, I’ll leave the optimized code in place, but I’ll probably delete it (and the requirement/test) next time I touch the Customer class.

So far, the design of the system hasn’t prevented me from being able to keep it running fast. I’ll make sure I revisit performance goals as I go.

Database TDD Part 20: Changing the Result Row to a Map

One thing I’m displeased with is the way that a query result gets mapped into the domain object.

   private static Column[] COLUMNS = new Column[] {
      new StringColumn(Customer.ID),
      new StringColumn(Customer.NAME),
      new IntegerColumn(Customer.BALANCE)
   };
   ...
   public Customer create(List<Object> row) {
      Customer customer = new Customer((String)row.get(0), (String)row.get(1));
      customer.charge((Integer)row.get(2));
      return customer;
   }

You know that the 0th element in a row is the customer ID only by correlating with the COLUMNS declaration at the top of CustomerAccess. This portends all sorts of maintenance nightmares that will occur when developers update the list of columns and forget to modify thecreate method, or vice versa. A better solution would be to have the query result returned as a map between column and value.

Here is the modified JdbcAccessTest:

package persistence;

import java.util.*;

import junit.framework.*;

public class JdbcAccessTest extends TestCase {
   private static final String TABLE = "JdbcAccessTest";
   private static final String COLUMN_NAME = "x";
   private static final String VALUE = "a";
   private static final Column[] COLUMNS = { new StringColumn(COLUMN_NAME) };

   private JdbcAccess access;

   protected void setUp() {
      access = new JdbcAccess();
      access.execute(String.format("create table %s (%s varchar(1))", TABLE,
            COLUMN_NAME));
   }

   protected void tearDown() {
      access.execute("drop table " + TABLE);
   }

   public void testExecuteQuery() {
      insertRow();
      assertResults(access.executeQuery(createSelectSql(), COLUMNS));
   }

   public void testExecuteQueryWithoutMetadata() {
      insertRow();
      assertResults(access.executeQuery(createSelectSql()));
   }

   public void testExecuteQueryNoResults() {
      assertNull(access.executeQuery(createSelectSql() + " where 1=0", COLUMNS));
   }

   private void assertResults(Map<String, Object> row) {
      assertEquals(1, row.size());
      assertEquals(VALUE, row.get(COLUMN_NAME));
   }

   private void insertRow() {
      String sql = String.format("insert into %s values('%s')", TABLE, VALUE);
      access.execute(sql);
   }

   private String createSelectSql() {
      return String.format("select %s from %s", COLUMN_NAME, TABLE);
   }
}

You’ll note that I refactored this code a good extent from the last state it was in. Once I had modified the executeQuery tests, I recognized lots of little problems that I cleaned up in a matter of minutes.

You might also note that there is no longer a method that explicitly tests the execute method. I chose to extract this test to its own class, JdbcAccessExecuteTest:

package persistence;

import java.util.*;

import junit.framework.*;

public class JdbcAccessExecuteTest extends TestCase {
   final String TABLE = "JdbcAccessTest";
   final String COLUMN_NAME = "x";
   private JdbcAccess access;

   protected void setUp() {
      access = new JdbcAccess();
   }

   public void testExecute() {
      try {
         access.execute(String.format("create table %s (%s varchar(1))", TABLE,
               COLUMN_NAME));
         assertEquals(0, getCount());
      } finally {
         access.execute(String.format("drop table %s", TABLE));
      }
   }

   private long getCount() {
      final String sql = String.format("select count(*) from %s", TABLE);
      Map<String, Object> row = access.executeQuery(sql);
      return ((Long)row.get("count(*)")).longValue();
   }
}

Putting testExecute into its own test class allowed for consistent symmetry between the setUp and tearDown methods in both test classes. The sole method in JdbcAccessExecuteTest does not require prior creation of a test table–creating the test table is the thing that’s getting tested. Each of the test methods in JdbcAccessTest do require that a test table get created and subsequently dropped after the test method completes.

The new preferred form of the executeQuery method, specified in the JdbcAccessTest method testExecuteQuery, takes a column array as its second argument. The name of each column is used as a key for putting each corresponding column value into the result row hash table.

...
public class JdbcAccess {
   ...
   public Map<String, Object> executeQuery(String sql) {
      return executeQuery(sql, null);
   }

   public Map<String, Object> executeQuery(String sql, Column[] columns) {
      try {
         createStatement();

         ResultSet results = statement.executeQuery(sql);
         Map<String, Object> row = null;
         if (results.next()) {
            if (columns == null)
               columns = createPseudoColumns(results.getMetaData());

            row = getRow(results, columns);
         }
         results.close();

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

   private Column[] createPseudoColumns(ResultSetMetaData metadata) throws SQLException {
      Column[] columns = new Column[metadata.getColumnCount()];
      for (int i = 1; i <= metadata.getColumnCount(); i++)
         columns[i - 1] = new StringColumn(metadata.getColumnName(i));
      return columns;
   }

   private Map<String, Object> getRow(ResultSet results, Column[] columns) throws SQLException {
      Map<String, Object> row = new HashMap<String, Object>();
      for (Column column : columns)
         row.put(column.getName(), results.getObject(column.getName()));
      return row;
   }
   ...
}

If no column metadata is provided, the executeQuery method simply constructs dummy columns from the result set’s metadata.

After I got this working, I moved back to the Persister class, which is the only place that the executeQuery method is called. As a more incremental, intermediary step, I chose to not change the PersistableMetadata interface, so that the create method (T create(List<Object> row)) still expects a list instead of a map. This allowed me to isolate changes for the time being to Persister and its test.

package persistence;

import java.util.*;

import junit.framework.*;

public class PersisterTest extends TestCase {
   private static final String TABLE = "x";
   private static final Object RETURN_OBJECT = "object";
   private static final String BAD_KEY = "not found";
   private static final String COLUMN1 = "a";
   private static final String COLUMN2 = "b";
   private static final String VALUE1 = "1";
   private static final String VALUE2 = "2";
   private static final Column[] COLUMNS = { new StringColumn(COLUMN1),
         new StringColumn(COLUMN2) };

   private JdbcAccess access;
   private String lastSql;
   private PersistableMetadata<Object> metadata;
   private Persister<Object> persister;

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

         public Map<String, Object> executeQuery(String sql, Column[] columns) {
            lastSql = sql;
            if (sql.indexOf(BAD_KEY) > -1)
               return null;
            Map<String, Object> results = new HashMap<String, Object>();
            results.put(COLUMN1, VALUE1);
            results.put(COLUMN2, VALUE2);
            return results;
         }
      };

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

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

         public Object create(List<Object> row) {
            if (row.get(0).equals(VALUE1) &amp;&amp; row.get(1).equals(VALUE2))
               return RETURN_OBJECT;
            return null;
         }

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

      persister = new Persister<Object>(metadata, access);
   }

   public void testSave() {
      Persistable persistable = new Persistable() {
         public Object get(String key) {
            if (key.equals(COLUMN1))
               return VALUE1;
            if (key.equals(COLUMN2))
               return VALUE2;
            return null;
         }
      };

      persister.save(persistable);

      String expectedSql = String.format(
            "insert into %s (%s,%s) values ('%s','%s')", TABLE, COLUMN1,
            COLUMN2, VALUE1, VALUE2);
      assertEquals(expectedSql, lastSql);
   }

   public void testFindBy() {
      final String key = VALUE1;
      assertEquals(RETURN_OBJECT, persister.find(key));
      String expectedSql = String.format("select %s,%s from %s where %s='%s'",
            COLUMN1, COLUMN2, TABLE, COLUMN1, key);
      assertEquals(expectedSql, lastSql);
   }

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

(You’ll note a bit of refactoring here too.) The setup for the test method testFindBy in PersisterTest had to change accordingly. I modified the executeQuery method override for JdbcAccess to create a map with forced results. The create method override for PersistableMetadata now ensures that the result row contains these appropriate results.

In Persister, I changed the find method to pass the column list into executeQuery. I then added the temporary code to push the values from the result row hashmap into the ordered list row (the row that gets passed to the metadata’s create method).

   public T find(String key) {
      String sql = new SqlGenerator().createFindByKey(metadata.getTable(),
            metadata.getColumns(), metadata.getKeyColumn(), key);
      Map<String, Object> row = access.executeQuery(sql, metadata.getColumns());
      if (row == null)
         return null;
      List<Object> tempRow = new ArrayList<Object>();
      for (Column column: metadata.getColumns())
         tempRow.add(row.get(column.getName()));
      return metadata.create(tempRow);
   }

The last set of steps is an incremental refactoring exercise.

  • Step 1: Add an overloaded create method (T create(Map row);) to the PersistableMetadata interface.
  • Step 2: Fix all compilation errors by implementing the interface where appropriate.
  • Step 3: Build the appropriate create method override code in PersisterTest.
  • Step 4: Modify Persister to call the new create method. This will break tests for UserAccess and CustomerAccess.
  • Step 5: Implement the new create method correctly in UserAccess and CustomerAccess. Ensure tests pass.
  • Step 6: Remove the old create method from PersistableMetadata and eliminate its implementations in PersisterTest, UserAccess, and CustomerAccess.

The final relevant bits of code:

PersistableMetadata

package persistence;

import java.util.*;

public interface PersistableMetadata&lt;T&gt; {
   String getTable();
   String getKeyColumn();
   T create(Map<String,Object> row);
   Column[] getColumns();
}

PersisterTest

   protected void setUp() {
      ...
      metadata = new PersistableMetadata<Object>() {
         ...
         public Object create(Map<String,Object> row) {
            if (row.get(COLUMN1).equals(VALUE1) && row.get(COLUMN2).equals(VALUE2))
               return RETURN_OBJECT;
            return null;
         }
      };

      persister = new Persister<Object>(metadata, access);
   }

Persister

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

UserAccess

   private static Column[] COLUMNS = new Column[] {
      new StringColumn(User.NAME),
      new StringColumn(User.PASSWORD)
   };

   public User create(Map<String,Object> row) {
      return new User((String)row.get(User.NAME), (String)row.get(User.PASSWORD));
   }

CustomerAccess

   private static Column[] COLUMNS = new Column[] {
      new StringColumn(Customer.ID),
      new StringColumn(Customer.NAME),
      new IntegerColumn(Customer.BALANCE)
   };

   public Customer create(Map<String,Object> row) {
      Customer customer =
         new Customer((String)row.get(Customer.ID), (String)row.get(Customer.NAME));
      customer.charge((Integer)row.get(Customer.BALANCE));
      return customer;
   }

I think the big lesson today is again one of controlled, incremental refactoring. At no time did I spend more than a couple minutes before I saw a green bar. I accomplished this in at least one case by introducing code that I knew would disappear shortly. The tradeoff is then one of guaranteed forward progress for slightly increased overall development time. Without taking this approach, I’m targeting potentially slightly diminished overall development time, but I run a risk of significantly increased overall development time if things go awry.

Database TDD Part 19: Incremental Refactoring

As I mentioned yesterday, I want to get rid of the notion of having separate arrays for a column’s name and its type. Today I underwent another dramatic refactoring, one that ended up taking about 30 minutes. I approached the refactoring very incrementally. The measure was always to ensure a green bar every 5 minutes, no more than 10. I ended up averaging a green bar about every two minutes.

Part of the trick to this incremental refactoring is to build up parallel implementations. Overloading methods and adding new methods are your first tactic. Once changed functionality is in place, you can go back and remove redundant implementations. After that’s done, you might find some automated renaming refactorings are in order (because the best names might have already been taken!).

The general thought today is that metadata should be represented as a collection of Column objects:

   private static Column[] COLUMNS = new Column[] {
      new StringColumn(Customer.ID),
      new StringColumn(Customer.NAME),
      new IntegerColumn(Customer.BALANCE)
   };

To support this, the definition of PersistableMetadata would need to change:

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

But I really didn’t take the big step of deleting the older methods right away (getColumns which returned a String array, and getTypeswhich returned an array of PersistableType). That happened only after everything was working with a new definition for getColumns, which I had temporarily named getAllColumns.

From there, the next goal was to drive changes into the Persister class. I updated PersisterTest and incrementally got both its save and findmethods working, without touching SqlGenerator. In order to do this, it meant writing translation code. For example, the save method calls SqlGenerator’s createInsert method. This method was still expecting a column names array and a types array. The translation code in Persister iterated the new Column[] and built both of these arrays.

After the translation code proved to work, I moved into SqlGenerator and incrementally refactored it to support taking an array of Column. Then it was a matter of working backward and refitting the call to SqlGenerator from Persister. Once that was done, I was able to tackle the fun part of deleting now-dead code.

The dead code included PersistableType. In the process of refactoring, the fact that each column needed to have a name and a type made me feel that an enum to represent type wasn’t necessarily the best idea. Why not just a simple polymorphic hierarchy instead?

ColumnTest

package persistence;

import junit.framework.*;

abstract public class ColumnTest extends TestCase {
   protected static final String NAME = "name";
   protected Column column;

   protected void setUp() {
      column = createColumn();
   }

   abstract protected Column createColumn();

   public void testCreate() {
      assertEquals(NAME, column.getName());
   }
}

Column

package persistence;

public interface Column {
   String getName();
   String sqlValue(Object object);
}

AbstractColumn

package persistence;

abstract public class AbstractColumn {
   private String name;

   public AbstractColumn(String name) {
      this.name = name;
   }

   public String getName() {
      return name;
   }
}

IntegerColumnTest

package persistence;

public class IntegerColumnTest extends ColumnTest {
   public void testSqlValue() {
      assertEquals("0", column.sqlValue(0));
   }

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

   @Override
   protected Column createColumn() {
      return new IntegerColumn(ColumnTest.NAME);
   }
}

IntegerColumn

package persistence;

public class IntegerColumn extends AbstractColumn implements Column {
   public IntegerColumn(String name) {
      super(name);
   }

   public String sqlValue(Object value) {
      if (value == null)
         return null;
      return value.toString();
   }
}

StringColumnTest

package persistence;

public class StringColumnTest extends ColumnTest {
   public void testSqlValue() {
      assertEquals("'a'", column.sqlValue("a"));
   }

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

   @Override
   protected Column createColumn() {
      return new StringColumn(ColumnTest.NAME);
   }
}

StringColumn

package persistence;

import util.*;

public class StringColumn extends AbstractColumn implements Column {
   public StringColumn(String name) {
      super(name);
   }

   public String sqlValue(Object input) {
      if (input == null)
         return null;
      return StringUtil.wrap(input.toString(), '\'');
   }
}

Note that a bunch of code from the PersistableType enum just moved across. I didn’t throw away the code, I recycled it. Also note use of the abstract test pattern, which helps ensure all superclass characteristics are consistently tested across all implementation subclasses.

So: a thirty minute exercise just to introduce a new type that I probably should have thought of at the outset? I can certainly justify the thirty minutes. In absence of having unit tests, I’ve done exercises like this in the past in one fell swoop. Mass change and replace, hack, whack, and 20 minutes later I have what I think is the problem. Only trouble is, I go to (manually) test it, and discover I did something stupid, or forgot something. In the process of even this incremental refactoring, I missed a couple of simple things; my tests told me immediately.

Old world result: 20 more minutes later, I might have come up with the proper solution, but I’d no longer bet on it. Nor would I bet on it being the correct solution.

As far as knowing ahead of time to use a column object? I might have. I also might have chosen the wrong level of abstraction. Doubtful here, but that’s not the real point.

In every system I’ve ever worked there have been numerous poor choices when it comes to design. I’ve also worked on systems where the initial design was pretty darn good. The team came up with a clean design that accommodated all of the requirements. Yet six months later, or two years later, someone came up with a new requirement that couldn’t be met easily with the existing design.

If you learn how to build systems using TDD and incremental redesign, you may find that you still end up down some of the same ratholes. You will spend more time getting to the better design than if you could have dreamed up that better design in the first place. But you’ll have the advantage in at least two ways: you’ll know a safe and sound approach for how to get from bad point A to good point B. You also will have spared the up-front effort that was wrong at least half of the time.

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.

Atom