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.

It's my site! Check out the About page for more about me, or follow me on Twitter at @jlangr.

Leave a Reply

*

captcha *

Atom