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 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 the create
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 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 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 executeQuery(String sql) {
return executeQuery(sql, null);
}
public Map executeQuery(String sql, Column[] columns) {
try {
createStatement();
ResultSet results = statement.executeQuery(sql);
Map 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 getRow(ResultSet results, Column[] columns) throws SQLException {
Map row = new HashMap();
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 metadata;
private Persister persister;
protected void setUp() {
access = new JdbcAccess() {
public void execute(String sql) {
lastSql = sql;
}
public Map executeQuery(String sql, Column[] columns) {
lastSql = sql;
if (sql.indexOf(BAD_KEY) > -1)
return null;
Map results = new HashMap();
results.put(COLUMN1, VALUE1);
results.put(COLUMN2, VALUE2);
return results;
}
};
metadata = new PersistableMetadata() {
public String getTable() {
return TABLE;
}
public String getKeyColumn() {
return COLUMNS[0].getName();
}
public Object create(List row) {
if (row.get(0).equals(VALUE1) && row.get(1).equals(VALUE2))
return RETURN_OBJECT;
return null;
}
public Column[] getColumns() {
return COLUMNS;
}
};
persister = new Persister(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 row = access.executeQuery(sql, metadata.getColumns());
if (row == null)
return null;
List tempRow = new ArrayList();
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
createmethod (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
createmethod override code in PersisterTest. -
Step 4: Modify Persister to call the new
createmethod. This will break tests for UserAccess and CustomerAccess. -
Step 5: Implement the new
createmethod correctly in UserAccess and CustomerAccess. Ensure tests pass. -
Step 6: Remove the old
createmethod 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<T> {
String getTable();
String getKeyColumn();
T create(Map row);
Column[] getColumns();
}
PersisterTest
protected void setUp() {
...
metadata = new PersistableMetadata() {
...
public Object create(Map row) {
if (row.get(COLUMN1).equals(VALUE1) && row.get(COLUMN2).equals(VALUE2))
return RETURN_OBJECT;
return null;
}
};
persister = new Persister(metadata, access);
}
Persister
public T find(String key) {
String sql = new SqlGenerator().createFindByKey(metadata.getTable(),
metadata.getColumns(), metadata.getKeyColumn(), key);
Map 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 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 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.
