[클린 코드] Ch.15 - JUnit

클릭하시면 네이버 Book으로 연결됩니다!

해당 서적을 참고하여 개인 공부용으로 정리한 글입니다.

---

JUnit

아래는 JUnit에서 문자열 비교 오류를 파악할 때 유용한 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 actual = 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 : "") + fExpected.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);
    }
}

전반적으로 잘 구성된 모듈이다.

하지만, 지금까지 공부한바로는 눈에 거슬리는 점이 있다.

보이스카우트 규칙에 따라 지금 코드보다 더 개선해보자.

불필요한 접두어

멤버 변수 앞에 접두어 f가 모두 붙어있다.

오늘날에는 변수 이름에 범위를 명시할 필요가 없다.

모두 제거한다.

표준 명명법 사용

접두어 f를 빼버리는 바람에 compactString 인수에 this가 붙었다.

멤버 변수와 지역 변수는 서로 다른 의미이므로 지역 변수명을 바꾼다.

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

조건문 캡슐화

if (fExpected == null || fActual == null || areStringsEqual()) {
    return Assert.format(message, fExpected, fActual);
}

의도를 명확히 표현하기 위해 조건문을 메서드로 뽑아내 적절한 이름을 붙인다.

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, expected, actual);
}

private boolean shouldNotCompact() {
    return expected == null || actual == null || areStringsEqual();
}

긍정문 사용

부정문보다 긍정문이 이해하기 쉽다.

shouldNotCompact()를 canBeCompacted()로 바꾸자.

private boolean canBeCompacted() {
    return expected != null && actual != null && !areStringsEqual();
}

이름으로 부수효과 설명

compact 메서드는 canBeCompacted 메서드가 false면 압축하지 않는다.

이름과 역할이 맞지 않는다.

이렇게 부가 단계가 숨겨지는 이름은 좋지 못하다.

이름은 함수, 변수, 클래스가 하는 일을 모두 담고 있는 것으로 사용해야 한다.

또한, 단순 문자열이 아닌 형식이 갖춰진 문자열을 반환하기 때문에 실제로는 formatCompactedComparison이라는 이름이 적합하다.

public String formatCompactedComparison(String message) { 
//public String compact(String message) {
    ...
}

한 가지만 할 것

if 문 안에서는 예상 문자열과 실제 문자열을 압축한다.

이 부분을 추출해 compactExpectedAndActual이라는 메서드로 만든다.

하지만 형식을 맞추는 것은 formatCompactedComparison에게 맡긴다. (역할 분리)

...

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 compactExpectedAndActual() {
    findCommonPrefix();
    findCommonSuffix();
    compactExpected = compactString(expected);
    compactActual = compactString(actual);
}

일관성 부족

compactExpectedAndActual 메서드에서 1,2행과 3,4행의 메서드 사용법이 일관적이지 못하다.

따라서 findCommonPrefix()와 findCommonSuffix()를 수정해 반환하게 만든다.

private compactExpectedAndActual() {
    prefixIndex = findCommonPrefix(); // findCommonPrefix();
    suffixIndex = findCommonSuffix(); // findCommonSuffix();
    compactExpected = compactString(expected);
    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.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;
}
...

동시에 멤버 변수 이름도 좀 더 정확하게 바꿨다.

숨겨진 시간적인 결합

findCommonSuffix를 잘 살펴보면 숨겨진 시간적인 결합(hidden temporal coupling)이 존재한다.

즉, findCommonSuffix는 findCommonPreffix가 prefixIndex를 계산한다는 사실에 의존한다.

그래서 두 메서드를 잘못된 순서로 호출하면 큰일난다.

따라서 시간 결합을 외부에 노출하고나 findCommonSuffix에 prefixIndex를 인수로 넘긴다.

private compactExpectedAndActual() {
    prefixIndex = findCommonPrefix();
    suffixIndex = findCommonSuffix(prefixIndex);
    String compactExpected = compactString(expected);
    String compactActual = compactString(actual);
}

private int findCommonSuffix(int prefixIndex) {
    ...
}

하지만 이 방법은 prefixIndex가 필요한 이유는 설명하지 못한다.

아래는 다른 방식으로 구현한 것이다.

private 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 >= prefix; 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.charAt(prefixIndex)) {
            break;
        }
    }
}

findCommonPrefixAndSuffix를 만들고 가장 먼저 findCommonPrefix를 호출한다.

이렇게 앞선 코드보다 순서가 더 분명해졌다.

 

이제 지저분한 코드를 정리해보자.

private void findCommonPrefixAndSuffix() {
    findCommonPrefix();
    int suffixLength = 1;
    for (; suffixOverlapsPrefix(suffixLength); suffixLength++) {
        if (charFromEnd(expected, suffixLength) != charFromEnd(actual, suffixLength)) {
            break;
        }
    }
    suffixIndex = suffixLength;
}

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;
}

suffixIndex가 실제로는 접미어 길이임이 드러났으므로 이름을 바꾼다.

경계 조건 캡슐화

computeCommonSuffix에 +1이 곳곳에 등장한다.

public class ComparisonCompactor {
    ...
    private int suffixLength;
    ...

    private void findCommonPrefixAndSuffix() {
        findCommonPrefix();
        suffixLength = 0;  // int suffixLength
        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);  //s.length() - i
    }

    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) + // - suffixIndex + 1
            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 + // - suffixIndex + 1
            contextLength, expected.length()); 
        return expected.substring(expected.length() - suffixLength, end) + // - suffixIndex + 1
            (expected.length() - suffixLength < // - suffixIndex + 1
                expected.length() - contextLength ? 
                    ELLIPSIS : "");
    }
}

computeCommonSuffix에서 +1을 없애고 charFromEnd에 -1을 추가하고 suffixOverlapsPrefix에 <=를 사용했다.

이후 suffixIndex를 suffixLength로 바꿨다.

그러나 +1을 제거하던 중 compactString에서 다음 행을 발견했다.

if (suffixLength > 0)

suffixLength가 이제 1씩 감소했으므로 >= 연산자로 고치는게 마땅해보이지만 아니다.

원래 코드가 버그라고도 할 수 있다.

분석한 결과 원래 코드는 언제나 suffixIndex가 1 이상이었으므로 if 문 자체가 무용지물이었다.

하지만, 이제는 길이가 0인 접미어를 걸러내 첨부하지 않는다.

 

같은 이유로 compactString 메서드에 있는 두 if문 역시 의심스러워 확인한 결과 필요없어 보여 제거했다.

private String compactString(String source) {
    return computeCommonPrefix() + DELTA_START 
    +  source.substring(prefixLength, source.length() - suffixLength)
    + DELTA_END + computeCommonSuffix();
}

---

최종 결과

이외에 자잘한 수정을 거쳐 탄생한 코드이다.

package junit.framework;

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 ComparisonCompactor(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 = comapct(expected);
            compactActual = comapct(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); suffixLength++) {
            if (charFromEnd(expected, suffixLength) != charFromEnd(actual, suffixLength)) {
                break;
            }
        }
    }

    private boolean suffixOverlapsPrefix(int suffixLength) {
        return actual.length() = suffixLength <= prefixLength || expected.length() - suffixLength <= prefixLength;
    }

    private void findCommonPrefix() {
        int prefixIndex = 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(startingEllipsis())
            .append(startingContext())
            .append(DELTA_START)
            .append(delta(s))
            .append(DELTA_END)
            .append(endingContext())
            .append(endingEllipsis())
            .toString();
    }

    private String startingEllipsis() {
        prefixIndex > 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() = suffixLength;
        int contextEnd = Math.min(contextStart + contextLength, expected.length());
        return expected.substring(contextStart, contextEnd);
    }

    private String endingEllipsis() {
        return (suffixLength > contextLength ? ELLIPSIS : "");
    }
}

모듈은 일련의 분석 함수와 일련의 조합 함수로 나뉜다.

코드를 잘 보면 초반에 내렸던 결정 일부를 번복했다.

리팩터링 시 흔히 생기는 일이며 더 좋은 코드를 위해 노력한 과정이다.