I always waver on the decision between creating duplication and then eliminating it, or staving it off immediately before you would have created it. I think the former is safer in terms of coming up with a better solution. It is, however, slower, something we’ll see today.
I wanted to force the issue of duplication by adding a second domain class, so as to figure out how the persistence driver was going to shake out. I created a new class Customer along with its associated test class. Copy and paste, literally. It’s a stupid data class, but let’s pretend it has legitimate behavioral capabilities that forced it into existence. The customer class contains an id and a name, both strings.
Same thing for Customer persistence needs: new CustomerAccessTest and CustomerAccess classes are pretty much copy-and-pasted. Is copying and pasting code bad? In my view, nothing’s bad until it’s checked in. Here it’s a tool to make the duplication explicit, visible in all its gory details, right in front of my face.
The alternative would be to factor relevant code from the UserAccess class. But I find it slightly more difficult to try to put all of that into my head. Here I can start with the obvious duplication and slowly refactor to an appropriate solution.
The first refactoring is to extract code that generates SQL into a completely separate class.
public class UserAccess {
private static final String TABLE_NAME = "userdata";
private static String[] columns = { "name", "password" };
public void save(User user) {
String sql = new SqlGenerator().createInsert(TABLE_NAME, columns, createValuesList(user));
new JdbcAccess().execute(sql);
}
private String createValuesList(User user) {
Transformer ticWrapper = new Transformer() {
public String transform(String input) {
return StringUtil.wrap(input, '\'');
}
};
return StringUtil.commaDelimit(new String[] { user.getName(),
user.getPassword() }, ticWrapper);
}
...
package persistence;
import util.*;
public class SqlGenerator {
public String createInsert(String tableName, String[] columns, String values) {
return String.format("insert into %s (%s) values (%s)", tableName,
createColumnList(columns), values);
}
private String createColumnList(String[] columns) {
return StringUtil.commaDelimit(columns);
}
}
The `createValuesList` method should obviously belong to in the
SqlGenerator class. The problem is how to get the user fields to the
SqlGenerator class such that it doesn’t create a backward dependency.
Easy enough: prepopulate the array of field values and pass it over.
~~~ java
// UserAccess code
public void save(User user) {
String[] fields = { user.getName(), user.getPassword() };
String sql = new SqlGenerator().createInsert(TABLE_NAME, columns, fields);
new JdbcAccess().execute(sql);
}
package persistence;
import util.*;
public class SqlGenerator {
public String createInsert(String tableName, String[] columns, String[] fields) {
return String.format("insert into %s (%s) values (%s)", tableName,
createColumnList(columns), createValuesList(fields));
}
private String createColumnList(String[] columns) {
return StringUtil.commaDelimit(columns);
}
private String createValuesList(String[] fields) {
Transformer ticWrapper = new Transformer() {
public String transform(String input) {
return StringUtil.wrap(input, '\'');
}
};
return StringUtil.commaDelimit(fields, ticWrapper);
}
}
The rest of the code refactoring is pretty straightforward, based on this direction. When I finished, my CustomerAccess class looked like this:
package domain;
import java.util.*;
import persistence.*;
public class CustomerAccess {
private static final String TABLE = "cust";
private static String[] COLUMNS = { "id", "name" };
public void save(Customer customer) {
String[] fields = { customer.getId(), customer.getName() };
String sql = new SqlGenerator().createInsert(TABLE, COLUMNS, fields);
new JdbcAccess().execute(sql);
}
public Customer find(String idKey) {
String sql = new SqlGenerator().createFindByKey(TABLE, COLUMNS, COLUMNS[0], idKey);
JdbcAccess access = new JdbcAccess();
List row = access.executeQuery(sql);
return new Customer(row.get(0), row.get(1));
}
}
This looks a like nicer, but it also still looks way too much like UserAccess. The fact that data is stored on the domain objects in explicit fields is going to continue to be a nuisance–unless I do something about that.
I’m essentially building metadata here, about the database tables and the domain objects for which they hold data. Where are these tables coming from? Right now, the presumption is that they already exist. What if I were to define the tables myself from code? Or what about the opposite–obtain all the appropriate data from the database itself? There are myriad ways to proceed from here; I’m going to need to decide on a direction soon. I’m willing to go in any direction–your input is welcomed.
For now, I’m going to finish cleanup on the code base, making some simple name changes and adding tests where needed.
Comments
Jeff Bay October 27, 2005 09:13am
My rule of thumb is to factor before introducing duplication if I can visualize in my head what it should look like after and I like the way it will look. If I can’t, then I duplicate it and then figure out how to remove the duplication.
Sometimes, I choose the former approach, and I’m wrong about my ability to visualize it, or I saw it incorrectly – if I can’t get back to passing test quickly, I start looking for a way to get back to passing tests by falling back to the duplicate first strategy.
I think you can save time and produce good solutions by factoring before adding behaviour – but there is a balance in how far you take it.
— JeffBay
And I’m not invisible, I’ve been signing my posts! :p Anonymous :
