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.