It’s been a long, wonderful holiday season. Sure, I had a couple of jobs, but I also had a good time, seeing family and friends from out east, and doing all sorts of fun holiday things. Unfortunately it meant I slacked off a bit with this blog. I’m picking it up today and want to get it to a point where I can consider it “good enough” to go to production with. Soon.
I looked at the code I’d built so far. So alien, so written by someone else, or so it seems. I picked around through tests to help re-familiarize myself with the code. But one of the ways I prefer to use to help understand code, old or new, mine or someone else’s, is to do a bit of refactoring on it. Through careful refactoring, I get a good sense of how things interrelate, while at the same time improving it.
In the database layer, I took a look at the sql package. This is the one that contains the SqlGenerate class and its attendant Criteria implementation classes (LikeCriteria, AndCriteria, etc.).
Well, one thing I’ve been doing lately is a lot of Fitnesse. Fitnesse combines a wiki with a simple acceptance testing framework. You express tests in wiki pages. Test pages thus have names like TestPatronFinesOnOverdueBook, or TestTransferHolding. A wiki word must composed of at least two separate words.
Jeff B. cornered me in the airport in Dallas and suggested that my naming in the database layer was too prolix. Of course I was mildly defensive, but I also admitted that wiki wording had tainted my Java coding. I was naming way too many of my identifiers with at least two words! More often than not, this was unnecessary.
In the sql package, I took a few minutes and renamed a number of things. For example:
-
AndCriteria to: And
-
Criteria.sqlString to: Criteria.sql
-
SqlGenerator.createColumnList to: SqlGenerator.columnList
-
SqlGenerator.createValuesList to: SqlGenerator.valuesList
And so on. This cleaned up things a good deal. I did not rename all of
the verbosely-named methods within SqlGenerator. For example, I
retainedcreateSelect, since I didn’t want to have an ambiguous method
named select. Is it a verb (action method) or noun (suggesting a query
or retrieval)? Maybe selectStatement would have worked, but it’s
equally wordy.
I then considered SqlGenerator. Right now, it’s virtually a utility class, since it has no attributes. All methods are data in, return value out. I may as well have made all the methods static. But maybe it should be an object, with attributes. Looking through the code, all SqlGenerator methods require a table name and a list of columns. Why not construct the object with those as arguments and save them in instance variables?
First I overloaded the constructor, so I now had two:
public class SqlGenerator {
private String table;
private Column[] columns;
public SqlGenerator(String table, Column[] columns) {
this.table = table;
this.columns = columns;
}
public SqlGenerator() {}
This allowed me to incrementally make my changes and not break gobs of client code all at once. Methods that had looked like:
public String createSelect(String tableName, Column[] columns, Criteria criteria) {
return String.format("%s where %s", createSelectAll(tableName, columns),
criteria.sqlString());
}
were that much simpler to read:
public String createSelect(Criteria criteria) {
return String.format("%s where %s", createSelectAll(), criteria.sql());
}
The tests got easier to read too.
I now had something close to “an object that represents SQL” than “an
object that generates SQL.” Why not just call the class Sql? And at that
point, I felt like it made more sense to have all the methods use a
consistent noun scheme. So I changed, for example, createSelect to
just select. There is an opportunity for modest confusion, but not for
anyone who has more than zero familiarity with the class.
Best of all, I like the way these changes cleaned up the client production code. The primary client of the SQL classes is Persister. Here’s what it now looks like:
Persister
package persistence;
import java.util.*;
import persistence.types.*;
import sql.*;
public class Persister {
private PersistableMetadata metadata;
private JdbcAccess access;
public Persister(PersistableMetadata metadata) {
this(metadata, new JdbcAccess());
}
public Persister(PersistableMetadata metadata, JdbcAccess access) {
this.metadata = metadata;
this.access = access;
}
public void save(T persistable) {
String sql = createSql().insert(
extractValues(persistable, metadata.getColumns()));
access.execute(sql);
}
private Sql createSql() {
return new Sql(metadata.getTable(), metadata.getColumns());
}
private Object[] extractValues(T persistable, Column[] columns) {
Object[] values = new Object[columns.length];
for (int i = 0; i < columns.length; i++)
values[i] = persistable.get(columns[i].getName());
return values;
}
public void saveAll(Collection collection) {
String sql = createSql().preparedInsert();
access.executeAll(sql, metadata.getColumns(),
createInsertRows(collection));
}
private List<Map<String, Object>> createInsertRows(Collection collection) {
List<Map<String, Object>> rows = new ArrayList<Map<String, Object>>();
for (T persistable : collection)
rows.add(createInsertRow(persistable));
return rows;
}
private Map<String, Object> createInsertRow(T persistable) {
Map<String, Object> row = new HashMap<String, Object>();
for (Column column : metadata.getColumns()) {
Object value = persistable.get(column.getName());
row.put(column.getName(), value);
}
return row;
}
public T find(String key) {
String sql = createSql().findByKey(metadata.getKeyColumn(), key);
Map<String, Object> row = access.executeQueryExpectingOneRow(sql,
metadata.getColumns());
if (row == null)
return null;
return metadata.create(row);
}
public List getAll() {
String sql = createSql().selectAll();
return executeQuery(sql);
}
public List findMatches(Column column, String pattern) {
String sql = createSql().select(column, pattern);
return executeQuery(sql);
}
public List find(Criteria criteria) {
return executeQuery(createSql().select(criteria));
}
private List executeQuery(String sql) {
List<Map<String, Object>> rows = access.executeQuery(sql, metadata
.getColumns());
List results = new ArrayList(rows.size());
for (Map<String, Object> row : rows)
results.add(metadata.create(row));
return results;
}
}
Hmm, should Persister have an instance variable of type Sql?
Feedback as always welcome. Particularly, what do you want to see next?
