When a design smell couldn't be more obvious

Today I was working on a small refactoring which I applied to a HttpServlet that exists in our shared codebase. This Servlet was written in a way which made it almost impossible to extend. A lot of private static methods, communicating with the database or accession filesystem resources, made this class really nice to test - ironically. In order to get anywhere, I initially decided to re-use the code by using the “copy-paste-change” pattern. A few weeks later I changed my mind and decided to make the shared code more extensible by refactoring the methods to be protected non-static. When I was done and I ran the testsuite, I saw that one test was failing.

Here is a snippet of a unit test that someone had written.


public class SomeTest {
private Method logMethod;

@BeforeMethod
public void setup() throws NoSuchMethodException {
logMethod = TheServlet.class.getDeclaredMethod(
"logEvent", new Class[] {
Parameter1.class,
Parameter2.class,
Parameter3.class}
);
}

@Test
public void testLogging() throws Exception {
logMethod.setAccessible(true);

// some set-up

logMethod.invoke(null, parameter1, parameter2, parameter3);

// asserts
}
}


Holy! I have seldom seen crappier code (and I am not even going to talk about having to catch Exception in the test method). Someone was getting a reference to the previously private static method inside the Servlet class, to be able to make it accessible via Reflection and invoke it from the test. Really, a design smell could not be more apparent. If you want to test the method, in that case it was doing some logging to the database, write a collaborator class to do the logging for you, that can be tested in isolation and is injected as a dependency. It is always good to take a step backwards and look at the million things you have to do in order to test stuff. This will most likely guide you to a better design of your software.