[클린코드] JUnit 들여다보기

이 글은 로버트 마틴의 '클린 코드'를 읽고 TIL(Today I Learned)로써 정리한 내용을 기록하는 글입니다.
자세한 내용은 책을 통해 확인하실 수 있습니다.

알라딘: 클린 코드 Clean Code (aladin.co.kr)

클린 코드 Clean Code

이번 장은 JUnit 프레임워크에서 ComparsionComparator 클래스를 가져와 리펙터링하는 과정을 설명합니다.
자세한 테스트 코드 및 과정은 책을 통해 확인해주시면 감사하겠습니다.

ComparisonCompactor

//원본 ComparisonCompactor 클래스.
public class ComparisonCompactor {
  private static final String ELLIPSIS = "...";
  private static final String DELTA_END = "]";
  private static final String DELTA_START = "[";
  
  private int fContextLength;
  private String fExpected;
  private String fActual;
  private int fPrefix;
  private int fSuffix;
  
  public ComparisonCompactor(int contextLength, String expected, String actual) {
    fContextLength = contextLength;
    fExpected = expected;
    fActual = actual;
  }
  
  public String compact(String message) {
    if (fExpected == null || fActual == null || areStringsEqual())
      return Assert.format(message, fExpected, fActual);
      
    findCommonPrefix();
    findCommonSuffix();
    String expected = compactString(fExpected);
    String acutal = compactString(fActual);
    return Assert.format(message, expected, actual);
  }
  
  private String compactString(String source){
    String result = DELTA_START + source.substring(fPrefix, source.length() - fSuffix + 1) + DELTA_END;
    if (fPrefix > 0)
      result = computeCommonPrefix() + result;
    if (fSuffix > 0)
      result = result + computeCommonSuffix();
    return result; 
  }
  
  private void findCommonPrefix() {
    fPrefix = 0;
    int end = Math.min(fExpected.length(), fActual.length());
    for(; fPrefix < end; fPrefix++) {
      if (fExpected.charAt(fPrefix) != fActual.charAt(fPrefix))
        break;
    }
  }
  
  private void findCommonSuffix() {
    int expectedSuffix = fExpected.length() - 1;
    int actualSuffix = fActual.length() - 1;
    for(; actualSuffix >= fPrefix && expectedSuffix >= fPrefix; actualSuffix--, expectedSuffix--) {
      if(fExpected.charAt(expectedSuffix) != fActual.charAt(actualSuffix))
        break;
    }
    fSuffix = fExpected.length() - expectedSuffix;
  }
  
  private String computeCommonPrefix() {
    return (fPrefix > fContextLength ? ELLIPSIS : "") + fExptected.substring(Math.max(0, fPrefix - fContextLength), fPrefix);
  }
  
  private String computeCommonSuffix() {
    int end = Math.min(fExpected.length() - fSuffix + 1 + fContextLength, fExpected.length());
    return fExpected.substring(fExpected.length() - fSuffix + 1, end) + (fExpected.length() - fSuffix + 1 < fExpected.length() - fContextLength ? ELLIPSIS : "");
  }
  
  private boolean areStringsEqual() {
    return fExpected.equals(fActual);
  }
}

• 비록 저자들이 모듈을 아주 좋은 상태로 남겨두았지만 보이스카우트 규칙에 따라 우리는 처음 왔을 때보다 더 깨끗하게 코드를 개선하고 떠나야 한다.

1차 개선

• 코드에서 가장 눈에 거슬리는 부분은 멤버 변수 앞에 붙인 접두어 f다.
• 오늘날 사용하는 개발 환경에서는 변수 이름에 범위를 명시할 필요가 없다. 접두어 f는 중복되는 정보이다.
• compact 함수 시작부의 조건문은 의도를 명확히 하기 위해 메서드로 캡슐화할 필요가 있다.

//1차 개선
public class ComparisonCompactor {
  private static final String ELLIPSIS = "...";
  private static final String DELTA_END = "]";
  private static final String DELTA_START = "[";
  
  private int contextLength;
  private String expected;
  private String actual;
  private int prefix;
  private int suffix;
  
  public ComparisonCompactor(int contextLength, String expected, String actual) {
    this.contextLength = contextLength;
    this.expected = expected;
    this.actual = actual;
  }
  
  public String compact(String message) {
    if (shouldNotCompact())
      return Assert.format(message, expected, actual);
      
    findCommonPrefix();
    findCommonSuffix();
    String expected = compactString(this.expected); 
    String acutal = compactString(this.actual);     
    return Assert.format(message, expected, actual);
  }
  
  private boolean shouldNotCompact() {
    return expected == null || actual == null || areStringsEqual();
  }
  
  private String compactString(String source){
    String result = DELTA_START + source.substring(prefix, source.length() - suffix + 1) + DELTA_END;
    if (prefix > 0)
      result = computeCommonPrefix() + result;
    if (suffix > 0)
      result = result + computeCommonSuffix();
    return result; 
  }
  
  private void findCommonPrefix() {
    prefix = 0;
    int end = Math.min(expected.length(), actual.length());
    for(; prefix < end; prefix++) {
      if (expected.charAt(prefix) != actual.charAt(prefix))
        break;
    }
  }
  
  private void findCommonSuffix() {
    int expectedSuffix = expected.length() - 1;
    int actualSuffix = actual.length() - 1;
    for(; actualSuffix >= prefix && expectedSuffix >= prefix; actualSuffix--, expectedSuffix--) {
      if(expected.charAt(expectedSuffix) != actual.charAt(actualSuffix)) break;
    }
    suffix = expected.length() - expectedSuffix;
  }
  
  private String computeCommonPrefix() {
    return (prefix > contextLength ? ELLIPSIS : "") + exptected.substring(Math.max(0, prefix - contextLength), prefix);
  }
  
  private String computeCommonSuffix() {
    int end = Math.min(expected.length() - suffix + 1 + contextLength, expected.length());
    return expected.substring(expected.length() - suffix + 1, end) + (expected.length() - suffix + 1 < expected.length() - contextLength ? ELLIPSIS : "");
  }
  
  private boolean areStringsEqual() {
    return expected.equals(actual);
  }
}

 

2차 개선

• 접두어 f를 삭제하면서 compact함수 내 지역변수 expected, acutal과 이름이 같은 멤버 변수가 되었다.
• 서로 다른 의미이므로 지역 변수의 이름을 명확하게 할 필요가 있다.
• 조건문을 캡슐화하였지만 함수 이름의 부정문은 긍정문보다 이해하기 어렵다. 긍정문으로 바꾸어 조건문을 반전한다.

//2차 개선
public class ComparisonCompactor {
  ...
  
  public String compact(String message) {
    if (canBeCompacted()){
      findCommonPrefix();
      findCommonSuffix();
      String compactExpected = compactString(expected); 
      String compactAcutal = compactString(actual);     
      return Assert.format(message, compactExpected, compactAcutal);
    } else {
      return Assert.format(message, expected, actual);    
    }
  }
  
  private boolean canBeCompacted() {
    return expected != null && actual != null && !areStringsEqual();
  }
  
  ...
}

 

3차 개선

• compact에는 오류 점검이라는 부가 단계가 숨어있다. (함수 이름은 compact이지만 canBeCompacted가 false이면 압축하지 않고 형식화를 한다.)
• compact함수는 단순히 압축된 문자열이 아닌 형식화된 문자열을 반환한다.
• 따라서 formatCompactedComparison이라는 이름이 적합하다.
• if문 내부의 압축하는 부분을 compactExpectActual함수로 만들어 호출하도록 하면 formatCompactedComparison 함수는 형식을 맞추는 작업만 수행할 수 있다.

//3차 개선
public class ComparisonCompactor {
  private String compactExpected;
  private String compactActual;
  ...
  
  public String formatCompactedComparison(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); 
    compactAcutal = compactString(actual);     
  }
  
  private boolean canBeCompacted() {
    return expected != null && actual != null && !areStringsEqual();
  }
  
  ...
}

 

4차 개선

• compactExpectedAndActual 함수를 만들었지만 반환 값이 없는 함수와 변수를 반환하는 함수를 섞어 사용하고 있다.
• 함수 사용방식이 일관적이지 못하므로 통일시킬 필요가 있다.
• prefix와 suffix는 색인 위치를 나타내는 멤버변수이다. prefixIndex, suffixIndex로 멤버 변수를 정확하게 바꾸어야 한다.

//4차 개선
public class ComparisonCompactor {
  ...
  
  private void compactExpectedAndActual() {
    prefixIndex = findCommonPrefix();
    suffixIndex = findCommonSuffix();
    compactExpected = compactString(expected); 
    compactAcutal = 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.charAt(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;
  }
  
  ...
}

 

5차 개선

• findCommonSuffix는 findCommonPrefix가 prefixIndex를 계산한다는 사실에 의존한다.
• 이는 숨겨진 시간적인 결합이며 잘못된 순서로 호출될 시 문제가 발생할 수 있다.

//5차 개선1 - 함수의 인수를 통해 의존관계를 표현
public class ComparisonCompactor {
  ...
  
  private void compactExpectedAndActual() {
    prefixIndex = findCommonPrefix();
    suffixIndex = findCommonSuffix(prefixIndex);
    compactExpected = compactString(expected); 
    compactAcutal = 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.charAt(prefixIndex))
        break;
    }
    return prefixIndex;
  }
  
  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;
  }
  
  ...
}

• prefixIndex를 인수로 전달하는 방식은 다소 자의적이다.
• 함수 호출순서는 확실히 정해졌지만 prefixIndex가 필요한 이유는 분명히 드러나지 않았다.
• 다른 프로그래머가 이러한 문제를 파악하여 원래대로 돌려놓을 수 있으므로 다른 방식을 고안해야한다.

//5차 개선2 - void형으로 함수를 선언하는 대신 호출 순서를 고정시키고 함수의 추상화 단계를 통일함.
public class ComparisonCompactor {
  ...
  
  private void compactExpectedAndActual() {
    findCommonPrefixAndSuffix();
    compactExpected = compactString(expected); 
    compactAcutal = compactString(actual);     
  }
  
  private void findCommonPrefixAndSuffix() {
    findCommonPrefix();
    int suffixLength = 1;
    for(; !suffixOverlapsPrefix(suffixLength); suffixLength++) {
      if(charFromEnd(expected, suffixLength) != charFromEnd(actual,suffixLength))
        break;
    }
    suffixIndex = suffixLength;
  }
  
  private void findCommonPrefix() {
    prefixIndex = 0;
    int end = Math.min(expected.length(), actual.length());
    for(; prefixIndex < end; prefixIndex++) {
      if (expected.charAt(prefixIndex) != actual.charAt(prefixIndex))
        break;
    }
  }
  
  private char charFromEnd(String s, int i) {
    return s.charAt(s.length()-i);
  }
  
  private boolean suffixOverlapsPrefix(int suffixLength) {
    return actual.length() - suffixLength < prefixLength || expected.length() - suffixLength < prefixLength;
  }
  ...
}

 

6차 개선

• 코드를 개선하고 나니 suffixIndex가 실제로는 접미어 길이라는 사실이 드러났다.
• 이름이 적절하지 못하고 length와 동의어가 되었으므로 length로 바꾸어야 한다.
• suffixLength는 1에서부터 시작한다. 그러므로 suffixLength는 진정한 길이가 아니다.
• computeCommonSuffix의 +1은 이를 고려하여 사용되고 있었으므로 수정할 필요가 있다.

//6차 개선 - computeCommonSuffix의 +1을 charFromEnd에 -1로 이동, suffixIndex를 suffixLength로 변경
public class ComparisonCompactor {
  ...
  private int suffixLength;
  ...
  
  private void findCommonPrefixAndSuffix() {
    findCommonPrefix();
    suffixLength = 0;
    for(; !suffixOverlapsPrefix(suffixLength); suffixLength++) {
      if(charFromEnd(expected, suffixLength) != charFromEnd(actual,suffixLength))
        break;
    }
  }
  
  private char charFromEnd(String s, int i) {
    return s.charAt(s.length()-i-1);
  }
  
  private boolean suffixOverlapsPrefix(int suffixLength) {
    return actual.length() - suffixLength <= prefixLength || expected.length() - suffixLength <= prefixLength;
  }
  
  ...
  
  private String compactString(String source) {
    String result = DELTA_START + source.substring(prefixLength, source.length() - suffixLength) + DELTA_END;
    if (prefixLength >= 0)
      result = computeCommonPrefix() + result;
    if(suffixLength >= 0)
      result = result + computeCommonSuffix();
    return result;
  }
  
  ...
  
  private String computeCommonSuffix() {
    int end = Math.min(expected.length() - suffixLength + contextLength, expected.length());
    return expected.substring(expected.length() - suffixLength, end) + (expected.length() - suffixLength < expected.length() - contextLength ? ELLIPSIS : "");
  }
}

최종 코드

• 코드를 조금씩 개선하여 다음과 같은 최종 코드를 작성하였다.
• 코드를 리펙터링 하다 보면 원래 했던 변경을 되돌리는 경우가 흔하다. (리펙터링은 코드가 어느 수준에 이를때까지 수많은 시행착오를 반복하는 작업이기 때문.)

//최종 코드
public class ComparisonCompactor {
  private static final String ELLIPSIS = "...";
  private static final String DELTA_END = "]";
  private static final String DELTA_START = "[";
  
  private int contextLength;
  private String expected;
  private String actual;
  private int prefixLength;
  private int suffixLength;
  
  public CompparsionCompactor(int contextLength, String expected, String actual) {
    this.contextLength = contextLength;
    this.expected = expected;
    this.actual = actual;
  }
  
  public String formatCompactedComparison(String message) {
    String compactExpected = expected;
    String compactActual = actual;
    if (shouldBeCompacted()) {
      findCommonPrefixAndSuffix();
      compactExpected = compact(expected);
      compactActual = compact(actual);
    }
    return Assert.format(message, compactExpected, compactActual);
  }
  
  private boolean shouldBeCompacted() {
    return !shouldNotBeCompacted();
  }
  
  private boolean shouldNotBeCompacted() {
    return expected == null || actual == null || expected.equals(actual);
  }
  
  private void findCommonPrefixAndSuffix() {
    findCommonPrefix();
    suffixLength = 0;
    for(; !suffixOverlapsPrefix(); suffixLength++) {
      if (charFromEnd(expected, suffixLength) != charFromEnd(actual, suffixLength))
        break;
    }
  }
  
  private char charFromEnd(String s, int i) {
    return s.charAt(s.length() - i - 1);
  }
  
  private boolean suffixOverlapsPrefix() {
    return actual.length() - suffixLength <= prefixLength || expected.length() - suffixLength <= prefixLength;
  }
  
  private void findCommonPrefix() {
    prefixLength = 0;
    int end = Math.min(expected.length(), actual.length());
    for(; prefixLength < end; prefixLength++)
      if(expected.charAt(prefixLength) != actual.charAt(prefixLength))
        break;
  }
  
  private String compact(String s) {
    return new StringBuilder()
      .append(startingEllippsis())
      .append(startingContext())
      .append(DELTA_START)
      .append(delta(s))
      .append(DELTA_END)
      .append(endingContext())
      .append(endingEllipsis())
      .toString();
  }
  
  private String startingEllipsis() {
    return prefixLength > contextLength ? ELLIPSIS : "";
  }
  
  private String startingContext() {
    int contextStart = Math.max(0, prefixLength - contextLength);
    int contextEnd = prefixLength;
    return expected.substring(contextStart, contextEnd);
  }
  
  private String delta(String s) {
    int deltaStart = prefixLength;
    int deltaEnd = s.length() - suffixLength;
    return s.substring(deltaStart, deltaEnd);
  }
  
  private String endingContext() {
    int contextStart = expected.length() - suffix.length;
    int contextEnd = Math.min(contextStart + contextLength, expected.length());
    return expected.substring(contextStart, contextEnd);
  }
  
  private String endingEllipsis() {
    return (suffixLength > contextLength ? ELLIPSIS : "");
  }
}