Writing Maintainable Code, Episode 1

Context:

This is in a medium-sized EJB-based application that uses bean managed persistence. An interface is defined to perform a set of searches of a certain type, with different implementing classes being used at runtime depending on what’s being searched for and what criteria are being used. Among the interface’s methods is one the returns a SQL sub-query based on a given collection of values. One implementation of that method is discussed here. The developer was awaiting help from the database group and never got back to this method to implement it. That didn’t cause any trouble at initial deployment, because this particular implementation wasn’t used by any of the code that could be called from the application’s client, though given that the decision as to what implementation will be used is made at runtime, that’s not apparent from the code.

Wrong:

    public String composeSqlStatement(Collection ruleValues) {
        //:TODO:
        //Talk to the DBAs and get help...
        return null;
    }

Result:

A few years after initial deployment, this application was to be significantly revised to reflect substantial changes in the business it supported, and the revision was to be done by an entirely new team of developers. In seeking to test new functionality, one of the developers wrote a unit test that, through the search interface, called the method above. However, since that method was only returning a piece of a larger SQL statement, the method calling it didn’t throw the NullPointerException that you’d expect with a method that’s meant to return a value returns null instead. When the resulting SQL statement was finally executed by another method elsewhere in the application code, an exception was thrown indicating that the SQL statement was invalid. Given that the SQL statement was more than a dozen lines long, it took some time to determine that the null, which appeared in the statement as an ordinary string (with a value of “null”), was causing the problem. And given that it was being called through an interface with more than a dozen implementing classes, it took more time to determine which method was returning the null. In all, it took more than one full day of developer time to resolve this.

Better:

    public String composeSqlStatement(Collection ruleValues) {
        //:TODO:
        //Talk to the DBAs and get help...
        throw new UnsupportedOperationException("composeSqlStatement()  " +
            "method not implemented in class TypeASearchDAO");
    }

Had the initial developer written the method to throw an exception if it was ever called, the problem would have been discovered the first time the method was called by the unit test. The cause of the problem and the appropriate solution would have been immediately apparent.