So Little Code - So Many Issues

ResultSet rs = pstatement.executeQuery();

boolean flag = false;



if (rs.next()) {

   if ("Y".equalsIgnoreCase(rs.getString(1)))

       ;

   {

       flag = true;

   }

}



if (flag) {

   // do something

}

How many problems can you find in the above code? I can think of a few.

The egregious issue is the stray semicolon after the condition check. Add two more brackets and it should become obvious what the issue is.

ResultSet rs = pstatement.executeQuery();

boolean flag = false;



if (rs.next()) {

   if ("Y".equalsIgnoreCase(rs.getString(2))) {

       ;

   }

   {

       flag = true;

   }

}



if (flag) {

   // do something

}

The semicolon is treated as a no-op and the conditional check treats that statement as to what should be executed if the condition is true. Because of this flag will always be set to true. I'm assuming that the developer intended that the flag only be set to true if the condition is true, but that is not what is happening.

The second issue may not cause a problem right away, but could easily cause an issue if the SQL query is changed in the future. Relying on the order of columns brought back by a query makes the code less readable and therefore more prone to future errors. Lets assume that the original query is something like:

select EMP_ID, ACTIVE_FLAG  from employees;

If someone comes along in the future and decides that we also need to retrieve the department number like so:

select EMP_ID, DEPT_NO, ACTIVE_FLAG from employees;

and changes the query, but forgets to change the index on the rs.getString(2), then we are going to have a big issue.

Rather than using the column index, it is preferable to use the column name:

rs.getString("ACTIVE_FLAG");

Another big issue is that multiple rows could be returned by the ResultSet, but the code assumes that we are only interested in the first row.

The hard-coded "Y" and bad variable name of 'flag' are also bad practices, but not as bad as the above.

Exploratory Testing: Did a Parameter Object Get Changed?

Exploratory testing is a useful way to find out how legacy code works. If you can write tests that exercise the legacy code in all its paths, you can then feel much safer in refactoring the code. If the refactored code still passes the tests, the refactoring is correct and has not created any side effects.

Sometimes parameter objects are modified inside methods. One way to test for this is to create two instances, pass one in to the method, and then compare the one passed in to see if it is equal to the unmodified one. This assumes that you have a correct implementation of equals().

public class LegacyTest {
    private Legacy legacy;
    private PassedIn passedIn;
    private PassedIn untouched;
    
    
    @Before
    public void setUp()
    throws Exception {
        legacy  = new Legacy();
        passedIn = new PassedIn();
        untouched = new PassedIn();
    }
    
    @Test
    public void passedInDidNotChange() {
        assertEquals(untouched, passedIn);
        legacy.method(passedIn);
        assertEquals(untouched, passedIn);
    }
}

Refactoring Code with Static Dependencies for Testability

Methods that have dependencies on static methods can be difficult to test. In this example we have a dependency that we obtain via a static method that we do not have control over and we want to test the surrounding code.

public void myMethod(Long id) {
     // ...
     Dependency dependency = DependencyUtil.getDependency();
     // ...
 }

We can refactor this to:

public void myMethod(Long id) {
     Dependency dependency = DependencyUtil.getDependency();
     myMethod(dependency);
 }

 void myMethod(Long id, Dependency dependency) {
     // ...
     doSomething(dependency);
     // ...
 }

We overload the method signature and preserve the public API. We make the new method have default access to make it testable. We can now mock Dependency however we like.

Bad Comments

Over the years I've found some 'interesting' comments in legacy code. One recent one is a seventeen line screed / excuse for how bad a particular piece of code is that ends with 'This makes me feel really dirty, fortunately, the Client/Server conversions have lowered the users expectations to the point where this seems inconsequential'.

A version history on this file shows that the commenter is the same person who wrote another comment that 'explains' why something is wrong because '... we are RETARDED' (referring to the organization). Supposedly this guy now works for Google. Amazing.

Indigestion from Digester and BeanUtils

We had some very simple XML that needed to be parsed. Apache Commons Digester was already in the project, so I used that and quickly whipped up a solution. All the happy-path tests worked, so I started adding edge-case tests.

A test that threw total nonsense malformed XML at it passed without any problems.

The next test was to pass in XML where one the attributes should have been a number, but the test passed a String. The test expected an Exception to be thrown, but there wasn't one and it failed. Surprisingly, Digester silently ignored the fact that the attribute could not be parsed into a long and simply left the attribute on the target class as zero.

Googling revealed two issues.

The first was that the default ErrorHandler for Digester swallows Exceptions. Bad stuff, but can be remedied by creating your own ErrorHandler:

ErrorHandler errorHandler = new ErrorHandler() {
 public void warning (SAXParseException e) throws SAXParseException { throw e; }
        public void error (SAXParseException e) throws SAXParseException { throw e; }
        public void fatalError(SAXParseException e) throws SAXParseException { throw e; }
};

Digester digester = new Digester();
digester.setErrorHandler(errorHandler);

This did not fix the test!

Further digging found that Digester depends on Apache Commons BeanUtils. In particular, it uses ConvertUtils to perform type conversions. The default converter is the culprit which silently does nothing if a conversion cannot be performed.

OK, so just right our own converter, right? Maybe.

You can right your own converter and you register it with ConvertUtils. But, the method signature for register is:

public static void register(Converter converter, Class clazz)

static!

This means that your custom converter will be used by all the Digester instances in your application. What's wrong with that? Well, in my case, this is a legacy application with test coverage around 1%. There may be code that 'depends' on the brokenness of the default converter behavior in Digester. Also, Digester is used by some other open source frameworks that may be in the project.

In the end, I decided not to use Digester and instead used dom4j which also allowed for a quick solution, but did the right thing and allowed me to control behavior on an instance by instance basis rather than using statics.

Commons-logging Headaches with Axis

I've written a web service using Axis 1.4 which depends on Apache commons-logging. Deploying on Tomcat, everything works fine, but some JUnit tests run fine in Eclipse, but break when running under CruiseControl throwing an exception:

org.apache.commons.discovery.DiscoveryException: No implementation defined for org.apache.commons.logging.LogFactory

The root cause is that commons-logging depends on Commons Discovery which plays tricks with the class-loader to 'discover' the logging implementation to use at runtime. For an exhaustive list of reasons as to why this is wrong-headed, read this article by Ceki Gülcü.

The normal way to specify which implementation to use is to have a commons-logging.properties file in the classpath with entries something like this:

org.apache.commons.logging.Log = org.apache.commons.logging.impl.Log4JLogger
org.apache.commons.logging.LogFactory = org.apache.commons.logging.impl.LogFactoryImpl

or to specify these as system properties using the -D option when running your app. In my case, I could not (easily) modify the ant targets to change the classpath because Configuration Management maintains shared build scripts that have to go through a heavy process to make any changes. To get around this, I set the system properties in code inside the test:

 @BeforeClass
 public static void beforeClass() {
  System.setProperty("org.apache.commons.logging.Log", "org.apache.commons.logging.impl.Log4JLogger");
  System.setProperty("org.apache.commons.logging.LogFactory", "org.apache.commons.logging.impl.LogFactoryImpl");
 }

This fixed the issue and the tests now run under CruiseControl.

Tapestry 5 with Run Jetty Run

I'm currently going through the Tapestry 5 tutorial and am quite impressed. However, the tutorial is a bit out of date and recommends using the Jetty Launcher Eclipse plug-in to launch Jetty and to only use Jetty 5 as Jetty Launcher does not support Jetty 6.

I could not get Jetty Launcher to work at all under Eclipse 3.4 (Ganymede) on Ubuntu Linux. The Jetty Launcher page notes that it is no longer being maintained and recommends trying Run Jetty Run.

After installing Run Jetty Run, everything seemed to be copacetic with class changes being loaded live without a server restart. That is, everything was OK until beaneditform component was added. At that point, the app bombed with:

Error obtaining injected value for field org.apache.tapestry5.corelib.components.Form.logger: java.lang.ClassNotFoundException: org.slf4j.Logger

Some googling led me to: Tapestry5HowToSetupEclipseRunJettyRun.

I could not really follow the directions about adding -Dorg.mortbay.jetty.webapp.parentLoaderPriority=true to the VM arguments for the launch configuration.

Finally I was able to get this to work by adding -Dorg.mortbay.jetty.webapp.parentLoaderPriority=true to the default JRE's VM arguments.

Go to Window -> Preferences -> Installed JREs. Select the default JRE and click the Edit button. Paste -Dorg.mortbay.jetty.webapp.parentLoaderPriority=true into the Default VM Arguments field and click the Finish button.

Code Syntax Highlighting

Here is a plug-in that formats code (Java, xml, etc) within html in a very nice way.
http://code.google.com/p/syntaxhighlighter/

This looks great. I'll be adding this functionality ASAP to handle code snippets.

Injecting a Spring Bean into a Servlet

I've done some searching on the web for how to cleanly inject a spring-managed bean into a servlet. There are a number of ways, but I believe this is the cleanest.

Assuming you have a bean, limaBean, that you want to inject into a servlet, here is the applicationContext.xml:

  <bean id="limaBean" class="com.example.LimaBeanImpl">

  <bean class="org.springframework.web.context.support.ServletContextAttributeExporter">
      <property name="attributes">
          <map>
              <entry key="limaBeanInServletContext">
                  <ref bean="limaBean">
              </ref>
          </entry></map>
      </property>
  </bean>

The ServletContextAttributeExporter takes an existing managed bean, 'limaBean', and injects it into the ServletContext under the name 'limaBeanInServletContext'.

The servlet accesses this bean like:

public class DocumentViewerServlet extends HttpServlet
{

  public void doGet(HttpServletRequest request, HttpServletResponse response)
      throws ServletException, IOException
  {
      LimaBean limaBean = getLimaBean();
      limaBean.doSomething();

     ...
  }

  private DocumentService getLimaBean()
  {
      ServletContext servletContext = this.getServletContext();

      return (LimaBean) servletContext
          .getAttribute("limaBeanInServletContext");
  }
}

At runtime, Spring will inject the bean and everything works.

The reason you wanted to inject the bean in the first place, though, was so that you could easily write a test that injects the bean.

Set the attribute on org.springframework.mock.web.MockServletConfig and pass that into the servlets init(ServletConfig) method.

public class VegetableServletTest extends TestCase
{
  private VegetableServlet servlet = null;
  private LimaBean fakeLimaBean = null;

  protected void setUp() throws Exception
  {
      super.setUp();

      servlet = new VegetableServlet();
      fakeLimaBean = new FakeLimaBean();

      MockServletConfig config = new MockServletConfig();
           config.getServletContext().setAttribute("limaBeanInServletContext", 
           fakeLimaBean);
   
      servlet.init(config);
   }

}

Cedric Beust vs Robert Martin

Cedric Beust recently launched a broadside attack on agile software development titled Agile People Still Don't Get It. Bob Martin returned fire today in a post strangely titled Agile People Still Don't Get It.

Cedric says that tests are not specs while Bob states that "a test is an unambiguous specification of behavior." I don't think that you can argue against the latter claim, but at the same time I don't think that a test is enough. There needs to be an agreed upon contract between the business folks and the developers as to what is going to be delivered. That contract is not going to be a unit test. I have yet to see a business person write a JUnit or TestNG test case, be able to read one or the other, or care to even try. Tests should be one means of meeting the contract.

Agile development practices are hands-down better at delivering projects on time, with lower defect rates, and with higher customer satisfaction. One of the core tenets is not to be afraid of changes in requirements. However, changes still need to be tracked. A check-in comment to the source control system and a unit test is not enough when the system has gone into production and someone claims that some feature is not working according as agreed.

This does not mean that everything has to be spec'd out before a line of code is written. Specs should evolve just like the code and tests.

I think that Martin bests Beust in the discussion of whether "If it's not testable, it's useless" is accurate.

Cedric Beust:

Software is shipped with untested parts every day, and just because it's not entirely tested doesn't mean it's bad software or that the untested parts are "useless".

Agilists just don't understand the meaning of calculated risk.

Robert Martin:

Here's the thing. If you can't test it, you don't know that it works. If you don't know that it works, then it's useless.

I feel like Beust is punting on this point and Martin is a bit too strict. There are things that are hard to test. The attempt should be made, but not at the cost of neglecting higher priority tasks.

Cedric then goes on to criticize agile developers as being smug and dishonest. I do not believe that agile developers are dishonest, but I have met a few who are very smug and I believe they, like Ruby proponents, could tone things down a notch.

Robert Martin shows real class in his summation and shows that Cedric Beust is wrong in thinking that all agile developers are rabid ideologues:

Agile is not a silver bullet. Agile is not THE ANSWER. But Agile techniques, and especially TDD, are very powerful techniques to get software written well, and to increase the level of professionalism in our industry.