Recently, I stumbled over the following code:
public boolean someMethod() {
List<String> list = getSomethingFromDatabase();
// something is done with the list
if (list != null && !list.isEmpty()) {
return true;
} else {
return false;
}
}Checkstyle and I agreed to get rid of the complicated if-clause. Due to the fact that I accidentaly broke another method during refactoring recently, I wanted to do a test-driven refactoring. To make things simple, there is a method in a utility class that is just right for this problem:
public static <T> boolean isEmptyOrNull(List<T> list) {
return (list == null || list.isEmpty());
}Obviously, this method can be used to simplify the code above. Maybe this is not a good example because the necessary change is obvious and easy. Nevertheless, it is a good exercise to practice some test-driven refactoring. So here is what I did, step by step.
1. Extract a method that only processes the logic
The first step of any of my refactorings is to isolate the code I want to refactor.
public boolean someMethod() {
List<String> list = getSomethingFromDatabase();
// do something with the list
return foo(list);
}
private boolean foo(List<String> list) {
if (list != null && !list.isEmpty()) {
return true;
} else {
return false;
}
}2. Tests
To guarantee that my refactoring doesn’t change the behavior of the code, I create a complete test-coverage in Line of Code (LOC) and of every logical path:
@Test
public void listIsNullTest() {
List<String> list = null;
boolean legacyResult = foo(list);
assertEquals(legacyResult, SomeUtilityClass.isEmptyOrNull(list));
}
@Test
public void listIsEmptyTest() {
List<String> list = new ArrayList<String>();
boolean legacyResult = foo(list);
assertEquals(legacyResult, SomeUtilityClass.isEmptyOrNull(list));
}
@Test
public void listIsNotEmptyTest() {
List<String> list = new ArrayList<String>();
list.add("one");
list.add("two");
boolean legacyResult = foo(list);
assertEquals(legacyResult, SomeUtilityClass.isEmptyOrNull(list));
}With that setting, all test are red because the logic has to be inverted (note the “!” before SomeUtilityClass.isEmpty):
@Test
public void listIsNullTest() {
List<String> list = null;
boolean legacyResult = foo(list);
assertEquals(legacyResult, !SomeUtilityClass.isEmptyOrNull(list));
}
@Test
public void listIsEmptyTest() {
List<String> list = new ArrayList<String>();
boolean legacyResult = foo(list);
assertEquals(legacyResult, !SomeUtilityClass.isEmptyOrNull(list));
}
@Test
public void listIsNotEmptyTest() {
List<String> list = new ArrayList<String>();
list.add("one");
list.add("two");
boolean legacyResult = foo(list);
assertEquals(legacyResult, !SomeUtilityClass.isEmptyOrNull(list));
}3. Final Refactoring
Now all tests are green, so the final refactoring can be made:
public boolean someMethod() {
List<String> list = getSomethingFromDatabase();
// do something with the list
return !SomeUtilityClass.isEmptyOrNull(list);
}Conclusion
The refactoring I made looks pretty complicated, considering such a tiny task. These baby-steps have two major advantages over a quick replace with the utility method. First, it is guaranteed to have the same logic. Second, the tests that have been written in the process of the refactoring will ensure the right behavior in the future if they are kept.