On July 3, 2011 wrote: A Small Exercise in Refactoring Data Access Objects
Dealing with “legacy” code is one of the problems I face most often on a day-to-day basis. Whilst it would be lovely to always be in a position to write clean, fresh code it is far more common that one will be in a position of having to adapt and modify pre-existing code that is far from ideal (which is often why it needs to be modified). Such code will commonly have no tests or any tests which do exist fail to test the code in any meaningful sense. Modifying such code can be fraught with peril: in the absence of good tests, the changes one makes can have more or less subtle repurcussions that can ripple across a system, frequently with adverse, unanticipated functional side-effects. Moreover, in order to implement new or modified functionality within such code utilising test-first methodologies, it is commonly first necessary to change the code in order to make it “testable”. Hence, at unit-test level, we have a kind of “Catch 22″ situation: I want to test the code in order to change it but first need to change it in order to test it. How do I ensure that my changes for the purposes of testability do not have a functional impact when those changes cannot themselves be tested? My answer is usually to “go up a level” and isolate the legacy code within an integration test harness. For the example of data access objects with which I am concerned here, this means actually going to the database. The rationale for this is simple: an integration test should prove the contracts of a class or method under runtime conditions. Integration tests may sometimes be slow but the confidence they provide to change the underlying code is essential in this case. Moreover, they serve to expose and document contracts which heretofore had probably never even been given consideration and this is a necessary step.
To begin with, one should be clear about the definitions of “legacy” and “refactoring” here:
- Legacy
- In line with many authors on TDD, such as Steve Freeman, I would initially define “legacy” as being simply “code without tests” (or code with meaningless tests, which is the same thing). Additionally, I would add the architectural definition that “legacy” code is that which utilises patterns/supporting technologies etc that are explicitly “deprecated” within the overall architectural plan for a system (you do have such a plan, don’t you?) — it has been identified and there is a strategy in place for its removal.
- Refactoring
- Consequently, refactoring can be defined as changing the underlying implementation for code that already passes good tests such that it is brought more fully into line with your overall architectural plan. If you change code without a test (I’m not so dogmatic as to say you should never do this, it is just that you need a very good reason), or before it is passing the test (a mistake when dealing with pre-existing code / simply the process of implementation for new code), then it is not refactoring.
Refactoring can be a contentious topic from a business perspective because, by definition, it should have no functional impact. The economic justifications are:
- Performance: the code works better under given runtime conditions.
- Maintainability: the code can be more quickly and easiliy modified under conditions of change to existing functionality.
- Extensibility: the code can be more quickly and easily modified given requirements for additional functionality.
Note that “bringing into line with overall architectural vision” is not a justification in-itself as this should just be a catch-all way of expressing the above points. (If not, what is your architecture achieving exactly?)
Anyway, moving on, I thought I would share with you a fairly straightforward case I had to deal with recently (note: names have been changed to protect the not-so-innocent!) I was given a story card describing some desired functional change from a use case perspective. Superficially, the change was simple, requiring only a change to some data “lookups” which were used to generate HTML select boxes for a user interface. Therefore, beginning with the relevant controller classes for the user interfaces that required the change, I was eventually confronted with the following class which is, to all intents and purposes, a data access object. It was being instantiated directly within a “view helper” class. Natch.
public class LegacyDAO extends AbstractLegacyDAO {
private static final String QUERY_TBL3 = "Select tbl3_id, tbl3_name from tbl3 where tbl3_boolean = 1 order by tbl3_name";
private static final String QUERY_WITH_JOIN =
"select distinct tbl1_id, " +
" tbl1_name,tbl1_boolean,tbl2_string " +
"from " + " tbl1, " + " tbl2 " +
"where " +
" tbl1_join_key = tbl2_join_key and " +
" (tbl2_some_id = 9005 or " +
" (tbl2_some_id = ? and tbl2_string in ('FOO','BAR'))) and " +
" tbl2_Start_date < sysdate and " +
" tbl2_end_date > sysdate " +
"order by" + " tbl1_name";
private List rowsFromTbl3;
private List hardCodedData;
private List rowsFromTbls1And2;
private List subsetOfRowsFromTbls1And2;
public List listSubsetOfRowsFromTbls1And2(int someId) {
try {
if (subsetOfRowsFromTbls1And2 == null) {
List allRows = listRowsFromTbls1And2(someId);
Iterator it = allRows.iterator();
while (it.hasNext()) {
KeyNameObject row = (KeyNameObject) it.next();
if (!row.getId().toUpperCase().endsWith("_SOMETHING")) {
if (subsetOfRowsFromTbls1And2 == null) {
subsetOfRowsFromTbls1And2 = new ArrayList();
}
subsetOfRowsFromTbls1And2.add(row);
}
}
}
} catch (Throwable e) {
throw new FullTraceException(e);
}
return subsetOfRowsFromTbls1And2;
}
public List listRowsFromTbls1And2(int someId) {
rowsFromTbls1And2 = new ArrayList();
Connection con = null;
PreparedStatement ps = null;
ResultSet rs = null;
try {
con = getDataSource().getConnection();
ps = con.prepareStatement(QUERY_WITH_JOIN);
ps.setInt(1,someId);
rs = ps.executeQuery();
while (rs.next()) {
int theId = rs.getInt("tbl1_id");
String name = rs.getString("tbl1_name");
String string = rs.getString("tbl2_string");
boolean bool = rs.getInt("tbl1_boolean") == 1;
if (string.toUpperCase().startsWith("FOO1")) {
string = "FOO1 FULL NAME";
} else if (string.equalsIgnoreCase("FOO2")) {
string = "FOO2 FULL NAME";
} else if (string.equalsIgnoreCase("FOO3")) {
string = "FOO3 FULL NAME";
} else if (string.equalsIgnoreCase("FOO4")) {
string = "FOO4 FULL NAME";
} else if (string.equalsIgnoreCase("FOO5")) {
string = "FOO5 FULL NAME";
} else {
string = null;
}
KeyNameObject row = new KeyNameObject();
String id = (new Integer(theId)).toString();
if (string != null) {
id = id + "_" + string;
} else {
id = id + "_DEFAULT FOO";
}
row.setId(id);
if (name.toLowerCase().startsWith("the")) {
String str = name.substring(0, "the".length());
name = name.substring("the".length() + 1, name.length()) + ", " + str;
}
if (string != null) {
name = name + " (" + string + ")";
} else {
name = name + " (DEFAULT FOO)";
}
row.setName(name);
boolean added = false;
for (int i = 0; i < rowsFromTbls1And2.size(); i++) {
KeyNameObject obj = (KeyNameObject) rowsFromTbls1And2.get(i);
if (row.getName().toUpperCase().compareTo(obj.getName().toUpperCase()) < 0) {
rowsFromTbls1And2.add(i, row);
added = true;
break;
}
}
if (!added) {
rowsFromTbls1And2.add(row);
}
}
} catch (Throwable e) {
throw new FullTraceException(e);
} finally {
try {
rs.close();
} catch (Throwable ignore) {
}
rs = null;
try {
ps.close();
} catch (Throwable ignore) {
}
ps = null;
try {
con.close();
} catch (Throwable ignore) {
}
con = null;
}
return rowsFromTbls1And2;
}
public List listRowsFromTbl3() {
if (rowsFromTbl3 != null) {
return rowsFromTbl3;
}
rowsFromTbl3 = new ArrayList();
Connection con = null;
PreparedStatement ps = null;
ResultSet rs = null;
try {
con = getDataSource().getConnection();
ps = con.prepareStatement(QUERY_TBL3);
rs = ps.executeQuery();
while (rs.next()) {
int id = rs.getInt("tbl3_id");
String name = rs.getString("tbl3_name");
KeyNameObject obj = new KeyNameObject();
obj.setId(String.valueOf(id));
obj.setName(name);
rowsFromTbl3.add(obj);
}
} catch (Throwable e) {
throw new FullTraceException(e);
} finally {
try {
rs.close();
} catch (Throwable ignore) {
}
rs = null;
try {
ps.close();
} catch (Throwable ignore) {
}
ps = null;
try {
con.close();
} catch (Throwable ignore) {
}
con = null;
}
return rowsFromTbl3;
}
public List listHardCodedData() {
if (hardCodedData != null) {
return hardCodedData;
}
hardCodedData = new ArrayList();
KeyNameObject role1 = new KeyNameObject();
role1.setName("NAME1");
hardCodedData.add(role1);
KeyNameObject role2 = new KeyNameObject();
role1.setName("NAME2");
hardCodedData.add(role2);
return hardCodedData;
}
@Override
protected String getPrimaryKeyQuery() {
return null;
}
@Override
protected Class getObjectClass() {
return LegacyDAO.class;
}
}
On seeing this, naturally, I groaned and the class was lit up like a Christmas tree by all the compiler warnings. I looked for the tests: of course, there were none. So I knuckled down and decided what to do about it. In line with my usual approach to such things, the broad strategy went like this:
- Read the code closely! We know what it is doing, because we know the user interface output, but how is it doing it? It has been in production for a long time, so we can safely assume that it must basically work, even if there are some known or currently-unknown “glitches”.
- Find all the things that are wrong about this class, both in simple terms (i.e outright bugs) and given the current architectural plan. Try to determine which parts of the code had simply become redundant since they were written (i.e. identify straightforward deletes).
- Determine basic plan for correcting the deficiencies and come up with a back-of-the-envelope estimate for how long each would take to complete.
- Given the budget and time constraints of the business change request driving the work, determine which of the changes would be appropriate within the scope of the current story card. Those that are too large can be marked for later completion either as independent technical debt story cards or subsequent functional change requests.
My notes ran as follows:
- Generally speaking, all each of the methods does is return query results mapped to a simple, generic
KeyNameObjectclass. It also hard-codes some manipulations of “name” values in order to produce the desired view output. The fact that it requires 200+ lines to achieve this is outrageous enough. Ideally, the desired view output names would be persistent values themselves and not be generated on read (this kind of “on read” manipulation is a failing I see very regularly: normalise your data on write, people!!) - It extends a deprecated support class (which I am here calling
AbstractLegacyDAO); a large class of which this class only uses thegetDataSourcemethod. Consequently, the class is compelled to “implement” 2 abstract methods from this class (getPrimaryKeyQueryandgetObjectClass), which it does incorrectly. We could, therefore, factor out this inheritance with ease, eradicating these pointless pseudo-implementations in the process. (Note: when you end up with pointless implementations like this, it is a clear sign that your inheritance is wrong, wrong, wrong.) - The data accessor methods lazily-initialise the results and “cache” them as instance members. The class is consequently not thread-safe. I then checked the number of times this class is used and these methods are called. Unsurprisingly, it turned out to be precisely 1. Hence the lazy-initialisation is a totally unnecessary “optimisation” that only increases the memory footprint of the class and increases complexity.
- No generics are used, since authorship of the class predated adoption of Java 1.5. At least strengthening of type safety in the API would be an easy win.
- Most of the class consists of boilerplate JDBC. Moreover, the data source is obtained (in the abstract superclass) via a static lookup. This could make testing difficult, except that I knew that testing of these static lookups had already been addressed as part of a move to Spring done some time back … at least, therefore, test harnesses were in-place already to deal with this. The adoption of Spring since this class was written meant that we could now also easily eradicate the boilerplate JDBC. We could, potentially, also make this DAO a Spring bean but, since neither the view helper (part of a Struts 1 app) nor the action had yet been Spring-ified, this would require some fairly extensive changes which would increase the time required for the work and its and change-impact.
- The on-read “name manipulations” were effectively describing a small enumeration. Complexity of the code could be reduced and readability improved by actually using one (i.e. an
enum)! - The “hard-coded data” method was patently absurd and did not even succeed in its own limited terms, containing an obvious copy & paste bug. Fortunately, on further investigation, it turned out to be completely unused. I silently hoped that had always been the case.
- The join query was sub-optimal and a quick explain plan showed that using a left join over a subselect would improve the performance of the query in this case.
- Both the queries and the code are full of magic numbers and strings. There is general plan in place to deal with this “facet” of the application, however resolving these issues in this case would be beyond the scope of the work as it would require changes in user interaction patterns amongst a number of other things, requiring the buy-in of the stakeholder.
- Why do people insist on writing their code in such an illegible manner (e.g. those static strings containing the SQL queries, with excessive concatentation make the queries almost unreadable [as well as being pointless, as they are only referenced once]) when, I would argue, it is as easy, of not easier, to achieve a consistent style. Also, at what point do you think the original author of this class might have stopped and begun to think that maybe, just maybe, 90 lines was a tad excessive for a simple data accessor method?! Clearly, s/he had never heard of “refactor into method”. A regular pattern of short, simple methods not only reduces complexity but also improves readability by virtue of the fact that humans are extremely adept at pattern recognition. What is often referred to as “beauty” (and too often dismissed by technicians as “mere aesthetics”) can, in fact, be described in such terms. The aesthetics of your code is not merely aesthetic: it is functional!
So, armed with this knowledge of the code, it was time to set about writing a good test that would ensure that, before I even contemplated introducing the requested functional changes (let alone starting refactoring), I could be sure that only (a) desired effects and not (b) undesired side-effects were the outcome of the work.
The process of writing such tests for “legacy” code can seem a little back-to-front because, rather than the usual “write test and change code till it passes” we instead “write test and keep changing it till it passes”. In other words: the code itself is the acceptance criteria of the test. We cannot change the code until our test passes. This often involves a process of “copying” or interpreting the existing implementation in test form. My test looked something like this:
public class LegacyDAOTest {
private LegacyDAO dao;
private JdbcTemplate db;
@Mock private BeanContainer container;
@Before public void setup() throws Exception {
initMocks(this);
Properties p = new Properties();
p.load(getClass().getResourceAsStream("/jdbc-test.properties"));
SimpleDriverDataSource ds = new SimpleDriverDataSource((Driver) Class.forName(p.getProperty("jdbc.driver")).newInstance(), p.getProperty("jdbc.url"), p.getProperty("jdbc.username"), p.getProperty("jdbc.password"));
when(container.get(DataSource.class)).thenReturn(ds);
StaticBeanFactory.setContainer(container);
db = new JdbcTemplate(ds);
dao = new LegacyDAO();
}
@After public void teardown() throws Exception {
StaticBeanFactory.setContainer(new DefaultContainer());
}
@Test public void shouldReturnSubsetOfRowsFromTbls1And2() throws Exception {
List<Integer> journals = db.queryForList("select some_id from some_table", Integer.class);
for (Integer id : journals) {
List<KeyNameObject> expected = getExpectedSubsetOfRowsFromTbls1And2(id);
List<KeyNameObject> actual = dao.listSubsetOfRowsFromTbls1And2(id);
assertThat(actual, equalTo(expected));
}
}
@Test public void shouldReturnRowsFromTbls1And2() throws Exception {
List<Integer> journals = db.queryForList("select some_id from some_table", Integer.class);
for (Integer id : journals) {
List<KeyNameObject> expected = getExpectedRowsFromTbls1And2(id);
List<KeyNameObject> actual = dao.listRowsFromTbls1And2(id);
assertThat(actual, equalTo(expected));
}
}
@Test public void shouldReturnRowsFromTbl3() throws Exception {
List<KeyNameObject> expected = db.query("select tbl3_id, tbl3_name from tbl3 where tbl3_boolean = 1 order by tbl3_name", new RowMapper<KeyNameObject>() {
@Override
public KeyNameObject mapRow(ResultSet rs, int rowNum) throws SQLException {
KeyNameObject kno = new KeyNameObject();
kno.setId(rs.getString("tbl3_id"));
kno.setName(rs.getString("tbl3_name"));
return kno;
}
});
List<KeyNameObject> actual = dao.listRowsFromTbl3();
assertThat(actual, equalTo(expected));
}
private List<KeyNameObject> getExpectedRowsFromTbls1And2(Integer journalId) {
List<KeyNameObject> expected = db
.query("select distinct tbl1_id, tbl1_name, tbl2_string from tbl1 join tbl2 on tbl2_join_key = tbl1_join_key where (tbl2_some_id = 9005 or (tbl2_some_id = ? and tbl2_string in ('FOO','BAR'))) and tbl2_start_date < sysdate and tbl2_end_date > sysdate order by tbl1_name",
new RowMapper<KeyNameObject>() {
@Override
public KeyNameObject mapRow(ResultSet rs, int rowNum) throws SQLException {
KeyNameObject o = new KeyNameObject();
String string = getMembershipType(rs.getString("tbl2_string"));
String id = rs.getString("tbl1_id");
String name = rs.getString("tbl1_name");
if (name.toLowerCase().startsWith("the")) {
String str = name.substring(0, "the".length());
name = name.substring("the".length() + 1, name.length()) + ", " + str;
}
o.setId(id + "_" + string);
o.setName(name + " (" + string + ")");
return o;
}
private String getMembershipType(String membershiptype) {
if (membershiptype.toUpperCase().startsWith("FOO1")) {
return "FOO1 FULL NAME";
} else if (membershiptype.equalsIgnoreCase("FOO2")) {
return "FOO2 FULL NAME";
} else if (membershiptype.equalsIgnoreCase("FOO3")) {
return "FOO3 FULL NAME";
} else if (membershiptype.equalsIgnoreCase("FOO4")) {
return "FOO4 FULL NAME";
} else if (membershiptype.equalsIgnoreCase("FOO5")) {
return "FOO5 FULL NAME";
} else {
return "DEFAULT FOO";
}
}
}, journalId);
Collections.sort(expected, new Comparator<KeyNameObject>() {
@Override
public int compare(KeyNameObject o1, KeyNameObject o2) {
return o1.getName().toUpperCase().compareTo(o2.getName().toUpperCase());
}
});
return expected;
}
private List<KeyNameObject> getExpectedSubsetOfRowsFromTbls1And2(Integer journalId) {
List<KeyNameObject> expected = db
.query("select distinct tbl1_id, tbl1_name, tbl2_string from tbl1 join tbl2 on tbl2_subscriber_id = tbl1_subscriber_id where (tbl2_some_id = 9005 or (tbl2_some_id = ? and tbl2_string in ('FOO','BAR'))) and tbl2_start_date < sysdate and tbl2_end_date > sysdate and not regexp_like(tbl2_string, '^.*_SOMETHING$') order by tbl1_name",
new RowMapper<KeyNameObject>() {
@Override
public KeyNameObject mapRow(ResultSet rs, int rowNum) throws SQLException {
KeyNameObject o = new KeyNameObject();
String string = getMembershipType(rs.getString("tbl2_string"));
String id = rs.getString("tbl1_id");
String name = rs.getString("tbl1_name");
if (name.toLowerCase().startsWith("the")) {
String str = name.substring(0, "the".length());
name = name.substring("the".length() + 1, name.length()) + ", " + str;
}
o.setId(id + "_" + string);
o.setName(name + " (" + string + ")");
return o;
}
private String getMembershipType(String membershiptype) {
if (membershiptype.equalsIgnoreCase("FOO2")) {
return "FOO2 FULL NAME";
} else if (membershiptype.equalsIgnoreCase("FOO3")) {
return "FOO3 FULL NAME";
} else if (membershiptype.equalsIgnoreCase("FOO4")) {
return "FOO4 FULL NAME";
} else if (membershiptype.equalsIgnoreCase("FOO5")) {
return "FOO5 FULL NAME";
} else {
return "DEFAULT FOO";
}
}
}, journalId);
Collections.sort(expected, new Comparator<KeyNameObject>() {
@Override
public int compare(KeyNameObject o1, KeyNameObject o2) {
return o1.getName().toUpperCase().compareTo(o2.getName().toUpperCase());
}
});
return expected;
}
}
As you can see, my test here replicates the existing implementation in most respects. I don’t try to do anything especially clever: the priority is to get some test code that accurately reproduces the output of the existing class as quickly as possible. The primary differences are:
- I use the improved query using a join in my test code and, for the method which obtains a subset of rows, I have a separate query that includes the required filtering clause in the query, so that I do not have to iterate over the result set twice in order to achieve this.
- I use existing test harness techniques (a mock
Container, using the wonderful Mockito) for theStaticBeanFactory… because it is static, I need to restore the default container in a teardown to ensure that I do not interfere with any subsequent tests. - I use
JdbcTemplateand extract the row mapping logic intoRowMappers (which were not available when the code was written, prior to the adoption of Spring — although I’m sure it would have been possible to conceive of one’s own alternative!) - To sort the results (which is done on the basis of the names as transformed in the code, so cannot be done in the query), I use a
Comparatorimplementation which I pass toCollections.sort(Collection, Comparator). This option was available when the code was written and is infinitely preferable to the frankly painful sight of watching some poor fool iterating over the results again and again trying to do this manually!
It is worth noting also that, in this test, the are effectively multiple assertions per-test because, for the sake of iron-clad confidence in the changes, I go through the entire table, testing against each available row identified by some_id (as I am calling it in my obfuscation). This, of course, makes the test take quite a long time to run. Before committing the test to the build, I spent a little time selecting a small number of critical cases and tested only those IDs because otherwise the build would have ground to a halt.
Given that this test passes (N.B. please excuse any errors I may have introduced whilst trying to obfuscate to protect the innocent — the real version works fine, I promise!), I then set about incrementally changing the implementations of the methods that I was going to retain through a series of IDE refactorings, supported by the introduction of a number of embedded classes aimed at removing duplication and reducing complexity:
@SuppressWarnings("synthetic-access") public class LegacyDAORefactored {
private static class CaseInsensitiveNameComaparator implements Comparator<KeyNameObject> {
@Override
public int compare(KeyNameObject o1, KeyNameObject o2) {
return o1.getName().toUpperCase().compareTo(o2.getName().toUpperCase());
}
}
private static enum MatchType {
TO_UPPER_CASE_STARTS_WITH {
@Override public boolean matches(String a, String b) {
return b != null && b.toUpperCase().startsWith(a);
}
},
EQUALS_IGNORE_CASE {
@Override public boolean matches(String a, String b) {
return a.equalsIgnoreCase(b);
}
};
public abstract boolean matches(String a, String b);
}
private static enum Foo {
FOO1_FULL_NAME("FOO1_DB_VALUE", MatchType.TO_UPPER_CASE_STARTS_WITH),
FOO2_FULL_NAME("FOO2", MatchType.EQUALS_IGNORE_CASE),
FOO3_FULL_NAME("FOO3", MatchType.EQUALS_IGNORE_CASE),
FOO4_FULL_NAME("FOO4", MatchType.EQUALS_IGNORE_CASE),
FOO5_FULL_NAME("FOO5", MatchType.EQUALS_IGNORE_CASE),
DEFAULT_FOO(null, null);
static Foo forString(String str) {
for (Foo foo : values()) {
if (!DEFAULT_FOO.equals(foo) && foo.matcher.matches(foo.string, str)) return foo;
}
return DEFAULT_FOO;
}
private final String string;
private final MatchType matcher;
private final String fullName;
private Foo(String string, MatchType matcher) {
this.string = string;
this.matcher = matcher;
fullName = name().replaceAll("_", " ");
}
public String getIdSuffix() {
return "_" + fullName;
}
public String getNameSuffix() {
return " (" + fullName + ")";
}
}
private static class KeyNameObjectRowMapper implements RowMapper<KeyNameObject> {
private static String putTheAtTheEnd(String name) {
if (name.toLowerCase().startsWith("the")) {
String str = name.substring(0, "the".length());
return name.substring("the".length() + 1, name.length()) + ", " + str;
}
return name;
}
@Override
public KeyNameObject mapRow(ResultSet rs, int rowNum) throws SQLException {
KeyNameObject kno = new KeyNameObject();
String string = rs.getString("tbl2_string");
Foo mt = Foo.forString(string);
kno.setId(rs.getString("tbl1_id") + mt.getIdSuffix());
kno.setName(putTheAtTheEnd(rs.getString("tbl1_name")) + mt.getNameSuffix());
return kno;
}
}
private static final RowMapper<KeyNameObject> RM = new KeyNameObjectRowMapper();
private static final Comparator<KeyNameObject> CMP = new CaseInsensitiveNameComaparator();
private final JdbcTemplate db;
public LegacyDAORefactored() {
db = new JdbcTemplate((DataSource) StaticBeanFactory.getBean(DataSource.class));
}
public List<KeyNameObject> listSubsetOfRowsFromTbls1And2(int someId) {
List<KeyNameObject> res = db.query("select distinct tbl1_id, tbl1_name, tbl2_string from tbl1 join tbl2 on tbl2_subscriber_id = tbl1_subscriber_id where (tbl2_some_id = 9005 or (tbl2_some_id = ? and tbl2_string in ('FOO','BAR'))) and tbl2_start_date < sysdate and tbl2_end_date > sysdate and not regexp_like(tbl2_string, '^.*_SOMETHING$') order by tbl1_name", RM, someId);
Collections.sort(res, CMP);
return res;
}
public List<KeyNameObject> listRowsFromTbls1And2(int someId) {
List<KeyNameObject> res = db.query("select distinct tbl1_id, tbl1_name, tbl2_string from tbl1 join tbl2 on tbl2_join_key = tbl1_join_key where (tbl2_some_id = 9005 or (tbl2_some_id = ? and tbl2_string in ('FOO','BAR'))) and tbl2_start_date < sysdate and tbl2_end_date > sysdate order by tbl1_name", RM, someId);
Collections.sort(res, CMP);
return res;
}
public List<KeyNameObject> listRowsFromTbl3() {
return db.query("select tbl3_id, tbl3_name from tbl3 where tbl3_boolean = 1 order by tbl3_name", new RowMapper<KeyNameObject>() {
@Override
public KeyNameObject mapRow(ResultSet rs, int rowNum) throws SQLException {
KeyNameObject kno = new KeyNameObject();
kno.setId(rs.getString("tbl3_id"));
kno.setName(rs.getString("tbl3_name"));
return kno;
}
});
}
}
Let’s go through some of the changes that were supported by the integration test, then …
- The
Comparatorimplementation required to do the final sorting of the result on the basis of mutated name values obtained from the database is used by more than one method. I therefore create a nested class for this and, because it is threadsafe, reference it via a static, final member on the DAO class. - The many
if ... else if ... elsestatements used to evaluate and transform the name values obtained from the database are encapsulated into a couple ofenumsusing a technique I am very fond of whereby we have an abstract method in the enum which allows each value to encapsulate small differences in logic. Here, for example, I have aMatcherenum which expresses the 2 different methods use to compare strings. I subsequently create aFooenum that contains the various name values I expect from the database. EachFoois instantiated with the requiredMatchertype. This, in turn, means that we can now … - Introduce a single
RowMapperimplementation shared by the 2 main data access methods (again, referenced via a static, final member on the DAO class) that can calculate the desired mutations of the name and ID values obtained from the database in a single statement, simply by callingFoo.forString(string_from_database). Nice. All the details of the enumerated values and their respective strategies for matching are encapsulated within theenums. I strongly recommend this way of using Java enums as a form of implementation of the strategy pattern wherever possible. It really is very neat. - We strongly-type the API by introducing generics into the collection return types. Quick and easy. (It also then becomes possible to subsequently remove some casts in calling code, which gives you the real gain here.)
- The very simple “table 3″ method is changed into a very standard
JdbcTemplatemethod with an inline row mapper. Again, a very easy change but one that really must be supported by the kind of test shown above to ensure that you don’t subtly alter the output in some unanticipated way. - The absurd and unused “hard coded data” method is simply removed with an IDE refactor — dont’t just delete it!
- The DAO no longer extends AbstractLegacyDAO, getting rid of one more dependency on a heavily deprecated class. The just-plain-wrong implementations of the inherited abstract methods from that class could consequently be removed, too.
- The class no longer “caches” the results of the query as instance members. Because we know that the class is only called once (and under no circumstances should we be adding new usages of this class), it is unecessary to do this and better that this is simply removed. So I removed it. Besides which, “embedding” the concern of “caching” directly in the DAO in this manner, aside from being done in an extremely poor way, is wrong: if it turns out later that we need this, it can be introduced as a cross-cutting concern using reliable and robust techniques.
Clearly, the changes that were made did not address many of the bigger architectural issues such as magic numbers, data structure issues that require manipulation of persistent values on-read, the fact that this DAO is instantiated and called only within a “view helper”, nor its obtaining a DataSource via a deprecated static lookup rather than being dependency injected. Nonetheless, I hope you agree that the refactored version of the code is, given the speed and ease of the changes (once the test was in place), considerably improved. Consequently, dealing with the bigger issues at a later date, under the auspices of work which would grant that scope, becomes feasible. Certainly, I found that changing the test in a simple way and subsequrntly introducing the requested functional changes was far easier and more reliable as a result. This is the everyday bread & butter of refactoring … improve, improve, improve, each time a class is modified so that you get you codebase into a position where it becomes possible to safely handle bigger and bigger issues, architecturally-speaking. Sometimes it can feel like you are getting nowhere and, if you have people in your team writing new crap faster than you can clean it up, then you can encounter problems. However, even a small team of good developers united and acting consistently in this way can effect remarkable improvements over even large codebases in a surpisingly short amount of time.
