Better refactoring... through testing

When you sit down to design a new class for your application, how many things do you normally intend it to do? When you’re finished implementing it… how many things does it actually do?

Are these different numbers?

I thought they might be. It’s far too easy—and I’m as guilty of this sin as the next programmer—to fold subtasks into a single class, because they’re all part of the same subsystem, right?

Wrong. Each class should focus on doing one thing, well. Subtasks need to be foisted off into a separate class that focusses on doing that subtask. There are two reasons for this:

1. You reduce coupling of the class. If you have a class A, that does subtasks a1, a2, and a3, and the subtasks don’t really affect one another, then class A doesn’t need to know everything about a1, a2, and a3.
2. You improve the testability of the class. Presumably the subtasks are all hidden behind private methods, because they shouldn’t be part of the parent class’s API. That much, at least, is correct. But if the subtasks are tightly coupled with the parent class, then you can’t readily test the subtasks without executing the entire parent task… and that may not be something you can express as a unit test, particularly if the parent task is dependent on an external Web service.
3. In addition to improving testability, you also make your tests meaningful. A failing test on a complex method can mean a lot of different things, depending on the nature of the failure—how was the result different from the expectation? If you have simple tests, that focus on short methods (you’re keeping your methods short, right), then when your tests fail (and you may get cascading failures), you’ll be able to identify fairly readily why they failed.

For the sake of a really facile, fairly stupid example, here’s a class that’s doing two entirely separate subtasks:

package com.prophecynewmedia.example.subtask;public class GodObject {  public String performsAComplexTask(String parameter1, String parameter2,                                     String parameter3, String parameter4) {    return combineTransformations(parameter1, parameter2, parameter3, parameter4);  }   private String combineTransformations(String parameter1, String parameter2,                                        String parameter3, String parameter4) {    return transformParametersAnotherWay(parameter1, parameter2)         + transformParametersOneWay(parameter3, parameter4);  }   private String transformParametersOneWay(String parameter1, String parameter2) {    return parameter1 + parameter2;  }   private String transformParametersAnotherWay(String parameter1, String parameter2) {    if (parameter1.contains(parameter2)) {      return "((" + parameter2 + ")" + parameter1 + ")";    }    return "(())";  }}

What do transformParametersOneWay() and transformParametersAnotherWay() have to do with each other, practically? They get called on different parameters, do different things, and are only combined in combineTransformations(). Here's the test class for GodObject:

package com.prophecynewmedia.example.subtask;import org.junit.Test;import static org.junit.Assert.assertEquals;public class GodObjectTest {  private GodObject godObject = new GodObject();  @Test public void confirmsLastTwoParametersCombineCorrectlyWithDifferentFirstParameters() {    assertEquals("(())cd", godObject.performsAComplexTask("a", "b", "c", "d"));  }   @Test public void confirmsFirstTwoParametersDoNotCombineCorrectly() {    assertEquals("(())", godObject.performsAComplexTask("a", "ab", "", ""));  }   @Test public void confirmsFirstTwoParametersCombineCorrectly() {    assertEquals("((a)ab)", godObject.performsAComplexTask("ab", "a", "", ""));  }}

Now, because of the simpleness of what we’re doing, this test doesn’t look so bad. Given. However, the first test also has to take into account what happens to the first two parameters in its expectations. This just isn’t clean. Surely there’s a way we can clean this up!

GodObject is doing a task that requires two completely distinct subtasks, and it&rsuqo;s doing those tasks too. In order to test one of those tasks, we have to take into account the results of the other. Let’s clean this up, one step a time…

Step 1: Make the first subclass.

We’ll modify GodObject.combineTransformations():

  private String combineTransformations(String parameter1, String parameter2, String parameter3, String parameter4) {    return transformParametersAnotherWay(parameter1, parameter2)         + SimpleTransformer.transform(parameter3, parameter4);  }   static public class SimpleTransformer {    static public String transform(String parameter1, String parameter2) {      return parameter1 + parameter2;    }  }

We’ll add a new test for this class:

package com.prophecynewmedia.example.subtask;import org.junit.Test;import static org.junit.Assert.assertEquals;public class SimpleTransformerTest { @Test public void combinesCorrectly() {  assertEquals("ab", GodObject.SimpleTransformer.transform("a", "b")); }}

So far, so good. We'll leave the first ugly test in the GodObjectTest, because it's the only test that confirms that combineTransformations() actually works. We'll rename it later. For now, let’s move that other transformation out into another new class.

Step 2: Make the second subclass

  ...  private String combineTransformations(String parameter1, String parameter2, String parameter3, String parameter4) {    return ComplexTransformer.transform(parameter1, parameter2)         + SimpleTransformer.transform(parameter3, parameter4);  }  ...   static public class ComplexTransformer {    public static String transform(String parameter1, String parameter2) {      if (parameter1.contains(parameter2)) {        return "((" + parameter2 + ")" + parameter1 + ")";      }      return "(())";    }  }

And the tests...

package com.prophecynewmedia.example.subtask;import org.junit.Test;import static org.junit.Assert.assertEquals;public class ComplexTransformerTest {  @Test public void confirmsParametersDoNotCombineCorrectly() {    assertEquals("(())", GodObject.ComplexTransformer.transform("a", "ab"));  }   @Test public void confirmsParametersCombineCorrectly() {    assertEquals("((a)ab)", GodObject.ComplexTransformer.transform("ab", "a"));  }}

This has made the last two tests in GodObjectTest redundant, because now we know that the transformations are being performed correctly (and will learn of failure if they change because the relevant tests will fail. I won’t bother showing the truncated test class here. It’s step 3, at any rate. Step 4 is moving the static inner classes into their own classes within the package.

There. We’ve now turned a facile, ugly, hard-to-test example into a facile, slightly less-ugly, easy-to-test, easy-to-refactor, easy-to-read-the-tests example. Huzzah! Being able to do this—to see where your classes are becoming too complex, and be able to refactor them into multiple simpler classes—is an important part of test-driven development, when you’re working with older code. If you inherit a project that has low test coverage, you may find yourself spending days doing this kind of stuff.

I’m tempted to set up Git and Subversion precommit hooks to prevent me from checking in code that isn’t tested. Granted, it would require parsing a coverage report and a diff, but it would probably pay off in the long run, because I’d know that everything I’m writing is tested, and would force me to leave the code better than I found it. It’s a story for another time, but we’re probably all familiar with the fear of refactoring another developer’s code in case you break it, and the inclination to just cobble on the changes you need to make. I know I am.