Graduate Program KB

Clean Code

JUnit Internals

• This chapter looks to critique a part of the JUnit implementation and improve upon it

• JUnit is a test automation framework for Java

• Listing 15-1 looks at ComparisonCompactorTest.java

  • The test suite covers every line of code or bit of functionality on ComparisonCompactor
  • Implies confidence in the code working and has been carefully refined by the developers

• Listing 15-2 looks at the implementation of ComparisonCompactor

  • Overall, clean and formatted code
  • Clear separation of variables, public and private methods
  • Highly cohesive methods
  • Few minor issues with naming and long expressions, as well as random "+ 1"s

• Boy Scout Rule tells us to leave the code cleaner than when we found it

  • Remove "f" prefix from variables
  • Issues with compact function
    • Unencapsulated conditional statement which doesn't convey clear intentions
    • Ambiguous variable names expected and actual exists as a member variable but also local variable, they represent different things

  public String compact(String message) {
      if (expected == null || actual == null || areStringsEqual())
          return Assert.format(message, expected, actual);
  
      findCommonPrefix();
      findCommonSuffix();
      String expected = compactString(this.expected);
      String actual = compactString(this.actual);
      return Assert.format(message, expected, actual);
  }
  

  public String compact(String message) {
      if (shouldNotCompact())
          return Assert.format(message, expected, actual);
  
      findCommonPrefix();
      findCommonSuffix();
      String compactExpected = compactString(expected);
      String compactActual = compactString(actual);
      return Assert.format(message, compactExpected, compactActual);
  }
  
  private boolean shouldNotCompact() {
      return expected == null || actual == null || areStringsEqual();
  }
  

  • Can also invert the conditional to handle the negative case last

  public String compact(String message) {
      if (canBeCompacted()) {
          findCommonPrefix();
          findCommonSuffix();
          String compactExpected = compactString(expected);
          String compactActual = compactString(actual);
          return Assert.format(message, compactExpected, compactActual);
      } else {
          return Assert.format(message, expected, actual);
      }
  }
  
  private boolean canBeCompacted() {
      return expected != null && actual != null && !areStringsEqual();
  }
  

  • Need to change function name, it might not compact the message if canBeCompacted returns false
    • Since the string returns a formatted string after possibly compacting, the name should be formatCompactedComparison
  • Compacting logic in if should be extracted to another function, de-coupling the formatting and compacting tasks

  public String compact(String message) {
      if (canBeCompacted()) {
          compactExpectedAndActual();
          return Assert.format(message, compactExpected, compactActual);
      } else {
          return Assert.format(message, expected, actual);
      }
  }
  
  private void compactExpectedAndActual() {
      findCommonPrefix();
      findCommonSuffix();
      compactExpected = compactString(expected);
      compactActual = compactString(actual);
      return Assert.format(message, compactExpected, compactActual);
  }
  

  • The prefix and suffix functions aren't consistent with the other two compact functions in returning variables
  • Modify the find functions to return the prefix and suffix values, also do a bit of variable renaming within them
  • The mentioned "+1"s are a result of suffixIndex and prefixIndex not being 0-based index, just change initialisation value from 1 to 0

• Following the Boy Scout Rule, the module has been improved despite it being in an already clean state

  • Essentially, no module can't be improved further