christophertownsondotcom

In ver­sion 2.5, released in Novem­ber 2007, Spring intro­duced annotation-based appli­ca­tion con­text con­fig­u­ra­tion ori­ented pri­mar­ily around the org.springframework.stereotype pack­age. Clearly, anno­ta­tions have been around in the Java and Spring world for quite some time now. Leav­ing aside the ques­tion of the pros and cons of anno­ta­tions them­selves for another time, it occurs to me that most devel­op­ers still tend to treat them as if they were a slightly mys­te­ri­ous form of code: they are reluc­tant to imple­ment their own and tend to use those that exist in only the most nor­ma­tive and mun­dane ways. There­fore, I would like to go through a very sim­ple exam­ple of how to take advan­tage of Spring’s stereo­type anno­ta­tions to sup­port and enhance your own appli­ca­tion architecture.

Out of the box, Spring pro­vides 4 basic archi­tec­tural stereo­types used com­monly by n-tier applications:

@Component

Used to indi­cate that the anno­tated class encap­su­lates a generic appli­ca­tion com­po­nent of an essen­tially unde­fined type.

@Repository

Used to indi­cate that the anno­tated class enca­pu­lates access to a sys­tem of record (e.g. a data accessor).

@Service

Used to indi­cate that the anno­tated class encap­su­lates a “ser­vice”; com­monly defined in terms of an API the under­ly­ing imple­men­ta­tion of which organ­ises sequences of lower-level tech­ni­cal logic into a call sequence within a trans­ac­tion bound­ary with the aim of deliv­er­ing a func­tional requirement.

@Controller

Used to indi­cate that the anno­tated class is a con­troller within an MVC application.

With the excep­tion of the @Component anno­ta­tion (which is the base type used to deter­mine that a class is a Spring bean dur­ing con­text ini­tial­i­sa­tion) and the @Controller anno­ta­tion (which is key when using Spring’s MVC sup­port), the spe­cial­i­sa­tions indi­cated serve lit­tle func­tional use on the whole. In most appli­ca­tions, it would be absolutely equiv­a­lent to mark a class as @Component in pref­er­ence to @Service, for exam­ple. How­ever, it need not be that way as not only can this meta­data be utilised for func­tional ends but also extend­ing these stereo­types pro­vides an excel­lent way to clar­ify the archi­tec­tural role of a given unit of code when none of the pre-existing spe­cial­i­sa­tions are appropriate.

For exam­ple, in my lit­tle hobby-project “Appo­site” (which I have men­tioned in pre­vi­ous posts), I have a num­ber of require­ments around ensur­ing state on appli­ca­tion startup (i.e. bootstrapping).

Now, boot­strap­ping is a clas­sic “non-functional” prob­lem (in so far as it usu­ally does not pro­vide func­tion­al­ity to an end user, merely ensur­ing that the appli­ca­tion is in an ade­quate state to be capa­ble of sub­se­quently deliv­er­ing this) … and, by no coin­ci­dence, it is also the kind of thing that there­fore also often gets left to the end and piled into some unread­able file that suf­fers from vir­u­lent bit-rot.

Alter­na­tively (and a slight improve­ment), the more organised-of-mind will retain the basic script­ing approach but struc­ture it via the use of the Tem­plate Pat­tern:

// abstract class with public *final* method defining algorithm, the individual methods for which may or may not also be abstract
public abstract class AbstractTemplatePattern {

    public final void doSomething() {
        doStepOne();
        Foo resultOfStepTwo = doStepTwo();
        doStepThree(resultOfStepTwo);
    }

    protected void doStepOne() {
        // maybe a default implementation here ...
    }

    protected abstracted Foo doStepTwo();

    protected abstracted void doStepThree(Foo foo);

}

How­ever, whilst this is slightly prefer­able, it is still not really ideal for the boot­strap­ping use case. This is because we expect the “algo­rithm” (i.e. “what needs to be done when boot­strap­ping”) to change with some fre­quency … this is usu­ally the cause of the main­te­nance night­mares that can some­times occur around such activ­i­ties. Whilst scripts can become illeg­i­ble under such cir­cum­stances, the tem­plate pat­tern becomes dif­fi­cult to main­tain because it is pre­cisely intended to rep­re­sent a sta­tic algo­rithm with chang­ing imple­men­ta­tions, whereas our boot­strap is the inverse: a chang­ing algo­rithm with sta­tic imple­men­ta­tions. In both cases, the code unit(s) involved are become sub­ject to change and, con­se­quently, bugs are liable to be intro­duced in the process of change. Asso­ci­ated tests may start to fail, and col­lab­o­ra­tors or sub­classes may be affected in all kinds of ways that are, to put it bluntly, a painful waste of time to deal with.

To talk in terms of pat­terns again, what we really want here are com­mands and a com­mand execu­tor. What we want to be able to do is sim­ply “drop-in” a new chunk of “stuff to do” and be com­fort­able in the knowl­edge that it will get done.

NB. It is not nec­es­sary to cre­ate your own inter­face for the com­mand pat­tern: java.lang.Runnable should always suf­fice, in my view. Use the stan­dard libraries when­ever possible!

We could, of course, use the JSR-250 @PostConstruct anno­ta­tion, sup­ported by Spring, wher­ever we wanted to do some kind of “boot­strap­ping”. How­ever, if we do so in an ad hoc fash­ion, (a) this logic becomes scat­tered through­out the code­base under generic meta­data that makes it more dif­fi­cult to iso­late in the process (b) frag­ment­ing this respon­si­bil­ity and (c) hin­der­ing re-use. There­fore, my approach was as follows …

First, cre­ate an org.apposite.command pack­age as the name­space for all code units that con­sti­tute a com­mand (i.e. they are an imple­men­ta­tion of java.lang.runnable and achieve some given tech­ni­cal require­ment (i.e. guar­an­tee state on com­ple­tion or throw an IllegalStateException on post-condition failure).

Sec­ond, extend the @Component stereo­type to intro­duce a new spe­cial­i­sa­tion, which I decided to call @StartupCommand:

@Target({ElementType.TYPE})
@Retention(RetentionPolicy.RUNTIME)
@Component
public @interface StartupCommand {

    String value() default "";
}

Sec­ond, imple­ment a java.lang.Runnable for each “thing” that needs to be done on startup. For exam­ple, in Appo­site, for var­i­ous rea­sons I will not bore you with here, I want to “reg­is­ter” the run­time instance that is in the pro­ces of start­ing up with a repository:

@StartupCommand public class RegisterRuntimeInstanceCommand implements Runnable {

    private final AppositeRuntimeInstanceDAO ridao;

    private final String riname;

    @Autowired public DetermineRuntimeInstanceCommand(@Value("${apposite.runtime.instance.name}") String instanceName, AppositeRuntimeInstanceDAO appositeRuntimeInstanceDAO) {
        riname = instanceName;
        ridao = appositeRuntimeInstanceDAO;
    }

    @Override public void run() {
        try {
            InetAddress localhost = InetAddress.getLocalHost();
            ridao.insertOrUpdate(riname, localhost.getHostName(), localhost.getHostAddress());
        } catch (Exception e) {
            throw new IllegalStateException("Runtime instance registration failure", e);
        }
    }

}

Now, for the com­mand execu­tor, we needn’t do any­thing nearly so fancy: we can encap­su­late our boos­t­rap logic (@PostConstruct) into a nor­mal Spring bean which, for the sake of argu­ment, I am going to call a boot­strap “service” …

public interface BootstrapService {

    void startup();

}

My imple­men­ta­tion of which is as follows:

@Service public class BootstrapSpringService implements BootstrapService {

    private static final Logger LOG = getLogger(BootstrapSpringService.class);

    private boolean bootstrap = true;

    private List<Runnable> startupCommands = new ArrayList<Runnable>();

    private final PlatformTransactionManager txmanager;

    @Autowired public BootstrapSpringService(PlatformTransactionManager transactionManager) {
        txmanager = transactionManager;
    }

    @Autowired public void setBootstrap(@Value("${apposite.bootstrap}") boolean bootstrap) {
        this.bootstrap = bootstrap;
    }

    @Autowired(required = false) public void setCommands(List<Runnable> commands) {
        for (Runnable command : commands) {
            if (AnnotationUtils.findAnnotation(command.getClass(), StartupCommand.class) != null) startupCommands.add(command);
        }
    }

    // NB. we cannot use @Transactional here because this is called *before* the creation of the transaction proxies
    @PostConstruct @Override public void startup() {
        if (bootstrap) {
            LOG.info("Bootstrapping Apposite ...");
            TransactionStatus tx = txmanager.getTransaction(new DefaultTransactionDefinition());
            for (Runnable command : startupCommands) {
                LOG.info("Running startup action: " + command.getClass().getName());
                long start = System.currentTimeMillis();
                command.run();
                LOG.info(command.getClass().getName() + " completed in " + (System.currentTimeMillis() - start) + "ms");
            }
            txmanager.commit(tx);
            LOG.info("Bootstrap complete.");
        }
    }

}

As a result, I can very eas­ily deter­mine what is sup­posed to hap­pen at startup and where. More­over, nei­ther the boot­strap “ser­vice” nor any of the indi­vid­ual com­mands will be sub­ject to change when, as I expect, addi­tional boot­strap logic is added in future. As a fringe ben­e­fit, the intent of the code is described with greater seman­tic force through the util­i­sa­tion of a spe­cific spe­cial­i­sa­tion annotation.

Deal­ing with “legacy” code is one of the prob­lems I face most often on a day-to-day basis. Whilst it would be lovely to always be in a posi­tion to write clean, fresh code it is far more com­mon that one will be in a posi­tion of hav­ing to adapt and mod­ify pre-existing code that is far from ideal (which is often why it needs to be mod­i­fied). Such code will com­monly have no tests or any tests which do exist fail to test the code in any mean­ing­ful sense. Mod­i­fy­ing such code can be fraught with peril: in the absence of good tests, the changes one makes can have more or less sub­tle repur­cus­sions that can rip­ple across a sys­tem, fre­quently with adverse, unan­tic­i­pated func­tional side-effects. More­over, in order to imple­ment new or mod­i­fied func­tion­al­ity within such code util­is­ing test-first method­olo­gies, it is com­monly first nec­es­sary to change the code in order to make it “testable”. Hence, at unit-test level, we have a kind of “Catch 22″ sit­u­a­tion: 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 pur­poses of testa­bil­ity do not have a func­tional impact when those changes can­not them­selves be tested? My answer is usu­ally to “go up a level” and iso­late the legacy code within an inte­gra­tion test har­ness. For the exam­ple of data access objects with which I am con­cerned here, this means actu­ally going to the data­base. The ratio­nale for this is sim­ple: an inte­gra­tion test should prove the con­tracts of a class or method under run­time con­di­tions. Inte­gra­tion tests may some­times be slow but the con­fi­dence they pro­vide to change the under­ly­ing code is essen­tial in this case. More­over, they serve to expose and doc­u­ment con­tracts which hereto­fore had prob­a­bly never even been given con­sid­er­a­tion and this is a nec­es­sary step.

To begin with, one should be clear about the def­i­n­i­tions of “legacy” and “refac­tor­ing” here:

Legacy

In line with many authors on TDD, such as Steve Free­man, I would ini­tially define “legacy” as being sim­ply “code with­out tests” (or code with mean­ing­less tests, which is the same thing). Addi­tion­ally, I would add the archi­tec­tural def­i­n­i­tion that “legacy” code is that which utilises patterns/supporting tech­nolo­gies etc that are explic­itly “dep­re­cated” within the over­all archi­tec­tural plan for a sys­tem (you do have such a plan, don’t you?) — it has been iden­ti­fied and there is a strat­egy in place for its removal.

Refac­tor­ing

Con­se­quently, refac­tor­ing can be defined as chang­ing the under­ly­ing imple­men­ta­tion for code that already passes good tests such that it is brought more fully into line with your over­all archi­tec­tural plan. If you change code with­out a test (I’m not so dog­matic as to say you should never do this, it is just that you need a very good rea­son), or before it is pass­ing the test (a mis­take when deal­ing with pre-existing code / sim­ply the process of imple­men­ta­tion for new code), then it is not refactoring.

Refac­tor­ing can be a con­tentious topic from a busi­ness per­spec­tive because, by def­i­n­i­tion, it should have no func­tional impact. The eco­nomic jus­ti­fi­ca­tions are:

1. Per­for­mance: the code works bet­ter under given run­time conditions.
2. Main­tain­abil­ity: the code can be more quickly and eas­iliy mod­i­fied under con­di­tions of change to exist­ing functionality.
3. Exten­si­bil­ity: the code can be more quickly and eas­ily mod­i­fied given require­ments for addi­tional functionality.

Note that “bring­ing into line with over­all archi­tec­tural vision” is not a jus­ti­fi­ca­tion in-itself as this should just be a catch-all way of express­ing the above points. (If not, what is your archi­tec­ture achiev­ing exactly?)

Any­way, mov­ing on, I thought I would share with you a fairly straight­for­ward case I had to deal with recently (note: names have been changed to pro­tect the not-so-innocent!) I was given a story card describ­ing some desired func­tional change from a use case per­spec­tive. Super­fi­cially, the change was sim­ple, requir­ing only a change to some data “lookups” which were used to gen­er­ate HTML select boxes for a user inter­face. There­fore, begin­ning with the rel­e­vant con­troller classes for the user inter­faces that required the change, I was even­tu­ally con­fronted with the fol­low­ing class which is, to all intents and pur­poses, a data access object. It was being instan­ti­ated 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 see­ing this, nat­u­rally, I groaned and the class was lit up like a Christ­mas tree by all the com­piler warn­ings. I looked for the tests: of course, there were none. So I knuck­led down and decided what to do about it. In line with my usual approach to such things, the broad strat­egy went like this:

1. Read the code closely! We know what it is doing, because we know the user inter­face out­put, but how is it doing it? It has been in pro­duc­tion for a long time, so we can safely assume that it must basi­cally work, even if there are some known or currently-unknown “glitches”.
2. Find all the things that are wrong about this class, both in sim­ple terms (i.e out­right bugs) and given the cur­rent archi­tec­tural plan. Try to deter­mine which parts of the code had sim­ply become redun­dant since they were writ­ten (i.e. iden­tify straight­for­ward deletes).
3. Deter­mine basic plan for cor­rect­ing the defi­cien­cies and come up with a back-of-the-envelope esti­mate for how long each would take to complete.
4. Given the bud­get and time con­straints of the busi­ness change request dri­ving the work, deter­mine which of the changes would be appro­pri­ate within the scope of the cur­rent story card. Those that are too large can be marked for later com­ple­tion either as inde­pen­dent tech­ni­cal debt story cards or sub­se­quent func­tional change requests.

My notes ran as follows:

• Gen­er­ally speak­ing, all each of the meth­ods does is return query results mapped to a sim­ple, generic KeyNameObject class. It also hard-codes some manip­u­la­tions of “name” val­ues in order to pro­duce the desired view out­put. The fact that it requires 200+ lines to achieve this is out­ra­geous enough. Ide­ally, the desired view out­put names would be per­sis­tent val­ues them­selves and not be gen­er­ated on read (this kind of “on read” manip­u­la­tion is a fail­ing I see very reg­u­larly: nor­malise your data on write, peo­ple!!)
• It extends a dep­re­cated sup­port class (which I am here call­ing AbstractLegacyDAO); a large class of which this class only uses the getDataSource method. Con­se­quently, the class is com­pelled to “imple­ment” 2 abstract meth­ods from this class (getPrimaryKeyQuery and getObjectClass), which it does incor­rectly. We could, there­fore, fac­tor out this inher­i­tance with ease, erad­i­cat­ing these point­less pseudo-implementations in the process. (Note: when you end up with point­less imple­men­ta­tions like this, it is a clear sign that your inher­i­tance is wrong, wrong, wrong.)
• The data acces­sor meth­ods lazily-initialise the results and “cache” them as instance mem­bers. The class is con­se­quently not thread-safe. I then checked the num­ber of times this class is used and these meth­ods are called. Unsur­pris­ingly, it turned out to be pre­cisely 1. Hence the lazy-initialisation is a totally unnec­es­sary “opti­mi­sa­tion” that only increases the mem­ory foot­print of the class and increases complexity.
• No gener­ics are used, since author­ship of the class pre­dated adop­tion of Java 1.5. At least strength­en­ing of type safety in the API would be an easy win.
• Most of the class con­sists of boil­er­plate JDBC. More­over, the data source is obtained (in the abstract super­class) via a sta­tic lookup. This could make test­ing dif­fi­cult, except that I knew that test­ing of these sta­tic lookups had already been addressed as part of a move to Spring done some time back … at least, there­fore, test har­nesses were in-place already to deal with this. The adop­tion of Spring since this class was writ­ten meant that we could now also eas­ily erad­i­cate the boil­er­plate JDBC. We could, poten­tially, also make this DAO a Spring bean but, since nei­ther the view helper (part of a Struts 1 app) nor the action had yet been Spring-ified, this would require some fairly exten­sive changes which would increase the time required for the work and its and change-impact.
• The on-read “name manip­u­la­tions” were effec­tively describ­ing a small enu­mer­a­tion. Com­plex­ity of the code could be reduced and read­abil­ity improved by actu­ally using one (i.e. an enum)!
• The “hard-coded data” method was patently absurd and did not even suc­ceed in its own lim­ited terms, con­tain­ing an obvi­ous copy & paste bug. For­tu­nately, on fur­ther inves­ti­ga­tion, it turned out to be com­pletely 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 sub­s­e­lect would improve the per­for­mance of the query in this case.
• Both the queries and the code are full of magic num­bers and strings. There is gen­eral plan in place to deal with this “facet” of the appli­ca­tion, how­ever resolv­ing these issues in this case would be beyond the scope of the work as it would require changes in user inter­ac­tion pat­terns amongst a num­ber of other things, requir­ing the buy-in of the stakeholder.
• Why do peo­ple insist on writ­ing their code in such an illeg­i­ble man­ner (e.g. those sta­tic strings con­tain­ing the SQL queries, with exces­sive con­ca­ten­ta­tion make the queries almost unread­able [as well as being point­less, as they are only ref­er­enced once]) when, I would argue, it is as easy, of not eas­ier, to achieve a con­sis­tent style. Also, at what point do you think the orig­i­nal author of this class might have stopped and begun to think that maybe, just maybe, 90 lines was a tad exces­sive for a sim­ple data acces­sor method?! Clearly, s/he had never heard of “refac­tor into method”. A reg­u­lar pat­tern of short, sim­ple meth­ods not only reduces com­plex­ity but also improves read­abil­ity by virtue of the fact that humans are extremely adept at pat­tern recog­ni­tion. What is often referred to as “beauty” (and too often dis­missed by tech­ni­cians as “mere aes­thet­ics”) can, in fact, be described in such terms. The aes­thet­ics of your code is not merely aes­thetic: it is functional!

So, armed with this knowl­edge of the code, it was time to set about writ­ing a good test that would ensure that, before I even con­tem­plated intro­duc­ing the requested func­tional changes (let alone start­ing refac­tor­ing), I could be sure that only (a) desired effects and not (b) unde­sired side-effects were the out­come of the work.

The process of writ­ing such tests for “legacy” code can seem a lit­tle back-to-front because, rather than the usual “write test and change code till it passes” we instead “write test and keep chang­ing it till it passes”. In other words: the code itself is the accep­tance cri­te­ria of the test. We can­not change the code until our test passes. This often involves a process of “copy­ing” or inter­pret­ing the exist­ing imple­men­ta­tion in test form. My test looked some­thing 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 repli­cates the exist­ing imple­men­ta­tion in most respects. I don’t try to do any­thing espe­cially clever: the pri­or­ity is to get some test code that accu­rately repro­duces the out­put of the exist­ing class as quickly as pos­si­ble. The pri­mary dif­fer­ences are:

• I use the improved query using a join in my test code and, for the method which obtains a sub­set of rows, I have a sep­a­rate query that includes the required fil­ter­ing clause in the query, so that I do not have to iter­ate over the result set twice in order to achieve this.
• I use exist­ing test har­ness tech­niques (a mock Container, using the won­der­ful Mock­ito) for the StaticBeanFactory … because it is sta­tic, I need to restore the default con­tainer in a tear­down to ensure that I do not inter­fere with any sub­se­quent tests.
• I use JdbcTemplate and extract the row map­ping logic into RowMappers (which were not avail­able when the code was writ­ten, prior to the adop­tion of Spring — although I’m sure it would have been pos­si­ble to con­ceive of one’s own alternative!)
• To sort the results (which is done on the basis of the names as trans­formed in the code, so can­not be done in the query), I use a Comparator imple­men­ta­tion which I pass to Collections.sort(Collection, Comparator). This option was avail­able when the code was writ­ten and is infi­nitely prefer­able to the frankly painful sight of watch­ing some poor fool iter­at­ing over the results again and again try­ing to do this manually!

It is worth not­ing also that, in this test, the are effec­tively mul­ti­ple asser­tions per-test because, for the sake of iron-clad con­fi­dence in the changes, I go through the entire table, test­ing against each avail­able row iden­ti­fied by some_id (as I am call­ing it in my obfus­ca­tion). This, of course, makes the test take quite a long time to run. Before com­mit­ting the test to the build, I spent a lit­tle time select­ing a small num­ber of crit­i­cal cases and tested only those IDs because oth­er­wise the build would have ground to a halt.

Given that this test passes (N.B. please excuse any errors I may have intro­duced whilst try­ing to obfus­cate to pro­tect the inno­cent — the real ver­sion works fine, I promise!), I then set about incre­men­tally chang­ing the imple­men­ta­tions of the meth­ods that I was going to retain through a series of IDE refac­tor­ings, sup­ported by the intro­duc­tion of a num­ber of embed­ded classes aimed at remov­ing dupli­ca­tion and reduc­ing 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 sup­ported by the inte­gra­tion test, then …

1. The Comparator imple­men­ta­tion required to do the final sort­ing of the result on the basis of mutated name val­ues obtained from the data­base is used by more than one method. I there­fore cre­ate a nested class for this and, because it is thread­safe, ref­er­ence it via a sta­tic, final mem­ber on the DAO class.
2. The many if ... else if ... else state­ments used to eval­u­ate and trans­form the name val­ues obtained from the data­base are encap­su­lated into a cou­ple of enums using a tech­nique I am very fond of whereby we have an abstract method in the enum which allows each value to encap­su­late small dif­fer­ences in logic. Here, for exam­ple, I have a Matcher enum which expresses the 2 dif­fer­ent meth­ods use to com­pare strings. I sub­se­quently cre­ate a Foo enum that con­tains the var­i­ous name val­ues I expect from the data­base. Each Foo is instan­ti­ated with the required Matcher type. This, in turn, means that we can now …
3. Intro­duce a sin­gle RowMapper imple­men­ta­tion shared by the 2 main data access meth­ods (again, ref­er­enced via a sta­tic, final mem­ber on the DAO class) that can cal­cu­late the desired muta­tions of the name and ID val­ues obtained from the data­base in a sin­gle state­ment, sim­ply by call­ing Foo.forString(string_from_database). Nice. All the details of the enu­mer­ated val­ues and their respec­tive strate­gies for match­ing are encap­su­lated within the enums. I strongly rec­om­mend this way of using Java enums as a form of imple­men­ta­tion of the strat­egy pat­tern wher­ever pos­si­ble. It really is very neat.
4. We strongly-type the API by intro­duc­ing gener­ics into the col­lec­tion return types. Quick and easy. (It also then becomes pos­si­ble to sub­se­quently remove some casts in call­ing code, which gives you the real gain here.)
5. The very sim­ple “table 3″ method is changed into a very stan­dard JdbcTemplate method with an inline row map­per. Again, a very easy change but one that really must be sup­ported by the kind of test shown above to ensure that you don’t sub­tly alter the out­put in some unan­tic­i­pated way.
6. The absurd and unused “hard coded data” method is sim­ply removed with an IDE refac­tor — dont’t just delete it!
7. The DAO no longer extends AbstractLe­ga­cy­DAO, get­ting rid of one more depen­dency on a heav­ily dep­re­cated class. The just-plain-wrong imple­men­ta­tions of the inher­ited abstract meth­ods from that class could con­se­quently be removed, too.
8. The class no longer “caches” the results of the query as instance mem­bers. Because we know that the class is only called once (and under no cir­cum­stances should we be adding new usages of this class), it is unec­es­sary to do this and bet­ter that this is sim­ply removed. So I removed it. Besides which, “embed­ding” the con­cern of “caching” directly in the DAO in this man­ner, aside from being done in an extremely poor way, is wrong: if it turns out later that we need this, it can be intro­duced as a cross-cutting con­cern using reli­able and robust techniques.

Clearly, the changes that were made did not address many of the big­ger archi­tec­tural issues such as magic num­bers, data struc­ture issues that require manip­u­la­tion of per­sis­tent val­ues on-read, the fact that this DAO is instan­ti­ated and called only within a “view helper”, nor its obtain­ing a DataSource via a dep­re­cated sta­tic lookup rather than being depen­dency injected. Nonethe­less, I hope you agree that the refac­tored ver­sion of the code is, given the speed and ease of the changes (once the test was in place), con­sid­er­ably improved. Con­se­quently, deal­ing with the big­ger issues at a later date, under the aus­pices of work which would grant that scope, becomes fea­si­ble. Cer­tainly, I found that chang­ing the test in a sim­ple way and sub­se­qurntly intro­duc­ing the requested func­tional changes was far eas­ier and more reli­able as a result. This is the every­day bread & but­ter of refac­tor­ing … improve, improve, improve, each time a class is mod­i­fied so that you get you code­base into a posi­tion where it becomes pos­si­ble to safely han­dle big­ger and big­ger issues, architecturally-speaking. Some­times it can feel like you are get­ting nowhere and, if you have peo­ple in your team writ­ing new crap faster than you can clean it up, then you can encounter prob­lems. How­ever, even a small team of good devel­op­ers united and act­ing con­sis­tently in this way can effect remark­able improve­ments over even large code­bases in a surpis­ingly short amount of time.