Test-Driven Refactoring


Posted by Steven

Recently, I stumbled over the following code:

  1. public boolean someMethod() {
  2. List<String> list = getSomethingFromDatabase();
  3.  
  4. // something is done with the list
  5.  
  6. if (list != null && !list.isEmpty()) {
  7. return true;
  8. } else {
  9. return false;
  10. }
  11. }

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:

  1. public static <T> boolean isEmptyOrNull(List<T> list) {
  2. return (list == null || list.isEmpty());
  3. }

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.

  1. public boolean someMethod() {
  2.  
  3. List<String> list = getSomethingFromDatabase();
  4.  
  5. // do something with the list
  6.  
  7. return foo(list);
  8. }
  9.  
  10. private boolean foo(List<String> list) {
  11. if (list != null && !list.isEmpty()) {
  12. return true;
  13. } else {
  14. return false;
  15. }
  16. }

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:

  1. @Test
  2. public void listIsNullTest() {
  3. List<String> list = null;
  4.  
  5. boolean legacyResult = foo(list);
  6. assertEquals(legacyResult, SomeUtilityClass.isEmptyOrNull(list));
  7. }
  8.  
  9. @Test
  10. public void listIsEmptyTest() {
  11. List<String> list = new ArrayList<String>();
  12.  
  13. boolean legacyResult = foo(list);
  14. assertEquals(legacyResult, SomeUtilityClass.isEmptyOrNull(list));
  15. }
  16.  
  17. @Test
  18. public void listIsNotEmptyTest() {
  19. List<String> list = new ArrayList<String>();
  20. list.add("one");
  21. list.add("two");
  22.  
  23. boolean legacyResult = foo(list);
  24. assertEquals(legacyResult, SomeUtilityClass.isEmptyOrNull(list));
  25. }

With that setting, all test are red because the logic has to be inverted (note the "!" before SomeUtilityClass.isEmpty):

  1. @Test
  2. public void listIsNullTest() {
  3. List<String> list = null;
  4.  
  5. boolean legacyResult = foo(list);
  6. assertEquals(legacyResult, !SomeUtilityClass.isEmptyOrNull(list));
  7. }
  8.  
  9. @Test
  10. public void listIsEmptyTest() {
  11. List<String> list = new ArrayList<String>();
  12.  
  13. boolean legacyResult = foo(list);
  14. assertEquals(legacyResult, !SomeUtilityClass.isEmptyOrNull(list));
  15. }
  16.  
  17. @Test
  18. public void listIsNotEmptyTest() {
  19. List<String> list = new ArrayList<String>();
  20. list.add("one");
  21. list.add("two");
  22.  
  23. boolean legacyResult = foo(list);
  24. assertEquals(legacyResult, !SomeUtilityClass.isEmptyOrNull(list));
  25. }

3. Final Refactoring

Now all tests are green, so the final refactoring can be made:

  1. public boolean someMethod() {
  2.  
  3. List<String> list = getSomethingFromDatabase();
  4.  
  5. // do something with the list
  6.  
  7. return !SomeUtilityClass.isEmptyOrNull(list);
  8. }

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.

Category: 
Share: