<<Clean Code>> Quotes: 15. JUnit Internals

It is simple in conception, precise in definition, and elegant in implementation.

The JUnit Framework

The first thing I don’t care for is the f prefix for the member variables. Today’s environments make this kind of scope encoding redundant.

Next, we have an unencapsulated conditional at the beginning of the compact function.

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

This conditional should be encapsulated to make our intent clear.

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

There shouldn’t have variables in the function that have the same names as the member variables.

  String compactExpected = compactString(expected);
  String compactActual = compactString(actual);

Negatives are slightly harder to understand than positives.

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

Naming this function compact hides the side effect of the error check. Notice also that the function returns a formatted message, not just the compacted strings.

public String formatCompactedComparison(String message) {

We should extract the body of the if statement as a method named compactExpectedAndActual. However, we want the formatCompactedComparison function to do all the formatting. The compact… function should do nothing but compacting.

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

Notice that this required us to promote compactExpected and compactActual to member variables. I don’t like the way that the last two lines of the new function return variables, but the first two don’t. They aren’t using consistent conventions. So we should change findCommonPrefix and findCommonSuffix to return the prefix and suffix values.

private void compactExpectedAndActual() {
  prefixIndex = findCommonPrefix();
  suffixIndex = findCommonSuffix();
  String compactExpected = compactString(expected);
  String compactActual = compactString(actual);
}
private int findCommonPrefix() {
  int prefixIndex = 0;
  int end = Math.min(expected.length(), actual.length());
  for(; prefixIndex < end; prefixIndex++) {
    if (expected.charAt(prefixIndex) != actual.chartAt(prefixIndex))
      break;
  }
  return prefixIndex;
}
private int findCommonSuffix() {
  int expectedSuffix = expected.length() - 1;
  int actualSuffix = actual.length() - 1;
  for(; actualSuffix >= prefixIndex && expectedSuffix >= prefixIndex; actualSuffix--, expectedSuffix--) {
    if (expected.charAt(expectedSuffix) != actual.charAt(actualSuffix))
      break;
  }
  return expected.length() - expectedSuffix;
}

Careful inspction of findCommonSuffix exposes a hidden temporal coupling; itdepends on the fact that prefixIndex is calculated by findCommonPrefix. If these two functions were called out of order, there would be a difficult debugging session ahead. So to expose this temporal coupling, let’s have findCommonSuffix take the prefixIndex as an argument.

private void compactExpectedAndActual() {
  prefixIndex = findCommonPrefix();
  suffixIndex = findCommonSuffix(prefixIndex);
  String compactExpected = compactString(expected);
  String compactActual = compactString(actual);
}
private int findCommonSuffix(int prefixIndex) {
  int expectedSuffix = expected.length() - 1;
  int actualSuffix = actual.length() - 1;
  for(; actualSuffix >= prefixIndex && expectedSuffix >= prefixIndex; actualSuffix--, expectedSuffix--) {
    if (expected.charAt(expectedSuffix) != actual.charAt(actualSuffix))
      break;
  }
  return expected.length() - expectedSuffix;
}

I’m not really happy with this. It works to establish the ordering but does nothing to explain the need for that ordering. Another programmer might undo what we have done because there’s no indication that the parameter is really needed. So let’s take a different tack.

private void compactExpectedAndActual() {
  findCommonPrefixAndSuffix();
  String compactExpected = compactString(expected);
  String compactActual = compactString(actual);
}
private void findCommonPrefixAndSuffix() {
  findCommonPrefix();
  int expectedSuffix = expected.length() - 1;
  int actualSuffix = actual.length() - 1;
  for(; 
      actualSuffix >= prefixIndex && expectedSuffix >= prefixIndex; 
      actualSuffix--, expectedSuffix--) {
    if (expected.charAt(expectedSuffix) != actual.charAt(actualSuffix))
      break;
  }
  suffixIndex = expected.length() - expectedSuffix;
}
private void findCommonPrefix() {
  int prefixIndex = 0;
  int end = Math.min(expected.length(), actual.length());
  for(; prefixIndex < end; prefixIndex++) {
    if (expected.charAt(prefixIndex) != actual.chartAt(prefixIndex))
      break;
  }
}