This morning: a five-minute refactoring to further eliminate duplication in the code. SQL statements are inherently redundant. They’re also risky to put together without a test against a live database.

Let’s hit the code. Here’s the entire User class:

    import java.util.*;
    
    public class User {
       private String name;
       private String password;
       private static final String TABLE_NAME = "userdata";
       private static String[] columns = { "name", "password" };
    
       public User(String name, String password) {
          this.name = name;
          this.password = password;
       }
    
       public String getName() {
          return name;
       }
    
       public String getPassword() {
          return password;
       }
    
       public void save() {
          new JdbcAccess().execute(String.format("insert into " + TABLE_NAME + " ("
                + User.createColumnList() + ") values ('%s', '%s')", name, password));
       }
    
       private static String createColumnList() {
          StringBuilder builder = new StringBuilder();
          for (int i = 0; i < columns.length; i++) { if (i > 0)
                builder.append(',');
             builder.append(columns[i]);
          }
          return builder.toString();
       }
    
       public static User find(String nameKey) {
          JdbcAccess access = new JdbcAccess();
          List row = access.executeQuery(String.format(
                "select " + createColumnList() + " from " + TABLE_NAME + " where name = '%s'",
                nameKey));
          return new User(row.get(0), row.get(1));
       }
    }

This was three refactoring passes, each concluded with the execution of all tests in the project. The first pass involved extracting the table name to a constant; 30 seconds. The second pass involved extracting the construction of the insert column list to a separate method that uses a String array of column names; 4 minutes. The third pass involved using createColumnList from the find method; 30 seconds.

The table name and the column lists were an obvious place to start. What about the values list in the insert statement? What about the duplication inherent in the User attributes themselves?

Another thought: the Single Responsibility Principle is getting more and more abused. It’s bad enough that the User domain class deals with persistence. Now there’s a new method createColumnList that is a generic String utility method. It belongs elsewhere. But don’t fret, we’ll get to that soon.

Comments

Anonymous October 17, 2005 at 05:47pm

Speaking of the single responsibility principle, how long until you make first class objects for Column and Table, and fold some of the sql creation logic into them?

–JeffBay

Anonymous October 17, 2005 at 05:49pm

And on another note, instead of “createColumnList”, or in addition to, the more generic “join” method would be a great addition to your string utilities handbook.

Strings.join(String\[\] strings, String delimeter)

createColumnList(columns) \{  
 return Strings.join(columns, “, “);  
}

–JeffBay

Jeff Langr October 17, 2005 06:25pm

Good comments, both. Not long. What I find interesting is that in 10 minutes worth of initial DB code, there’s so much refactoring that can be done.

Tonight, though, I’m almost asleep already. Maybe tomorrow night.

-j-

Share your comment