클린코드: 15. JUnit 들여다보기

JUnit 프레임워크 `ComparisonCompactor.java` 모듈 개선하기

생성자 두 번째 인수와 세 번째 인수를 비교한다.
comapct 함수에 문자열을 넣으면 결과 메시지 앞에 문자열이 추가된다.
비교 후 다른 문자열을 “[]”를 써서 강조한다.
첫 번째 인수는 다른 문자열을 출력할 때 “[]” 앞 뒤로 출력할 문자열 개수이다.

 1
 2
 3
 4
 5
 6
 7
 8
 9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
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 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는 중복되는 정보다.

1
2
3
4
5
private int contextLength;
private String expected;
private String actual;
private int prefix;
private int suffix;

캡슐화되지 않은 조건문

 1
 2
 3
 4
 5
 6
 7
 8
 9
10
11
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);
}

의도를 명확히 표현하려면 조건문을 캡슐화 하는것이 좋다. 조건문을 메서드로 뽑아내 적절한 이름을 붙인다.

 1
 2
 3
 4
 5
 6
 7
 8
 9
10
11
12
13
14
15
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.format(message, expected, actual);
}

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

중복되는 변수 이름

compact 함수에서 사용하는 this.expected와 this.acutal같은 경우 기존 함수에 expected, actual 이라는 지역 변수가 있었는데, 클래스 변수 fExpected, fAcutal에서 f를 제거하여 같은 이름을 사용하게 되었다.

함수에서 멤버 변수와 이름이 같은 변수를 사용하는 이유를 파악하고 더 서술적(구체적)인 이름으로 바꾼다.

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

조건문의 부정문 사용

부정문은 긍정문보다 이해하기 약간 더 어렵다. 첫 문장 if를 긍정으로 만들어 조건문을 반전한다.

 1
 2
 3
 4
 5
 6
 7
 8
 9
10
11
12
13
14
15
public String compact(String message) {
    if (**canBeCompacted**()) {
        return Assert.format(message, expected, actual);
    }

    findCommonPrefix();
    findCommonSuffix();
    String expected = compactString(this.expected);
    String actual = compactString(this.actual);
    return Assert.format(message, expected, actual);
}

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

동작, 의미를 잘 표현할 수 있는 이름을 사용

compact 함수는 문자열을 앞축하라는 의미가 강하지만 실제로 canBeCompacted가 false면 압축하지 않는다.
- compact라는 이름을 붙이면 오류 점검이라는 부가 단계(기능)이 숨겨진다.
함수는 단순히 압축된 문자열이 아니라 형식이 갖춰진 문자열을 반환한다.

실제로는 formatCompatedComparison이라는 이름이 적합하다.

새 이름에 인수를 고려하면 가독성이 훨신 좋아진다.

1
2
3
public String formatCompactedComparison(String message) {
...
}

함수가 한 가지 책임만 가지게

if 문 안에서는 예상 문자열과 실제 문자열을 진짜로 압축한다. 이 부분을 빼내 compactExpectedAndActual이라는 메서드로 만든다.

형식을 맞추는 작업은 formatCompactedComparison에게 전적으로 맡긴다.

 1
 2
 3
 4
 5
 6
 7
 8
 9
10
11
12
13
14
15
16
17
18
19
20
...
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);
}

compactExpected, compactActual를 멤버 변수로 승격했다.

사용 방식 일관성

새 함수compactExpectedAndActual에서 마지막 두 줄은 변수를 반환하지만 첫째 줄과 둘째줄은 반환값이 없다. 멤버변수 함수 사용방식이 일관적이지 못하다.

findCommonPrefix, findCommonSuffix는 함수 내부에서 멤버변수 prefix, suffix에 값을 할당하지만, compactString은 반환값을 만들게 되고, 멤버 변수에 반환값을 할당한다.

findCommonPrefix, findCommonSuffix 를 변경하여 접두어 값과 접미어 값의 위치를 반환하게 바꾼다.

 1
 2
 3
 4
 5
 6
 7
 8
 9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
private void compactExpectedAndActual() {
    prefixIndex = findCommonPrefix();
    suffixIndex = 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 를 주의 깊게 살펴보면 숨겨진 시간적인 결함이 존재한다. findCommonSuffix는 findCommonPrefix가 prefixIndex를 계산한다는 사실에 의존하게 된다.

만약 findCommonPrefix와 findCommonSuffix를 잘못된 순서로 호출하면 문제를 찾기 어렵다.

따라서 시간 결함을 외부에 노출하고자 findCommonSuffix를 고쳐 prefixIndex를 인수로 넘겼다.

 1
 2
 3
 4
 5
 6
 7
 8
 9
10
11
12
13
14
15
16
17
private void compactExpectedAndActual() {
    prefixIndex = findCommonPrefix();
    suffixIndex = findCommonSuffix(prefixIndex);
    compactExpected = compactString(expected);
    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;
}

prefixIndex를 인수로 전달하는 방식은 함수 호출 순서를 확실히 명시할 수 있지만, prefixIndex가 필요한 이유를 명확히 설명하지 못한다.

필요한 이유가 분명히 드러나지 않으므로 다른 개발자가 원래대로 되돌려놓을지도 모른다.

 1
 2
 3
 4
 5
 6
 7
 8
 9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
private void compactExpectedAndActual() {
    **findCommonPrefixAndSuffix**();
    compactExpected = compactString(expected);
    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() {
    prefixIndex = 0;
    int end = Math.min(expected.length(), actual.length());
    for (; prefixIndex < end; prefixIndex++)
        if (expected.charAt(prefixIndex) != actual.charAt(prefixIndex))
            break;
}

findCommonPrefix와 findCommonSuffix를 되돌리고, findCommonSuffix라는 이름을 findCommonPrefixAndSuffix로 바꾼 후 findCommonPrefixAndSuffix에서 가장 먼저 findComonPrefix를 호출한다.

두 함수를 호출하는 순서가 앞서 고친 코드보다 훨씬 더 분명해진다.

그리고 PrefixAndSuffix함수를 정리한다.

 1
 2
 3
 4
 5
 6
 7
 8
 9
10
11
12
13
14
15
16
17
18
19
20
21
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가 index가 아닌 실제로는 접미어 길이(length)를 의미한다. 이름이 적절치 않다. prefixIndex도 마찬가지이다.

 1
 2
 3
 4
 5
 6
 7
 8
 9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
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 : "");
    }
}

computeCommonSuffix에서 +1을 없애고 charFromEnd에 -1을 추가하고 suffixOverlapsPrefix에 ≤ 를 사용했다. 그 후 suffixLength로 바꿔 가독성을 크게 높힐 수 있었다.

불필요한 코드 제거

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

1
if (suffixlength > 0)

suffixLength가 1씩 감소했으므로 당연히 연산자를 ≥로 고쳐야 하지만 지금 상황에서는 ≥는 나올 수 없다.

코드를 좀 더 분석해보면 if 문은 길이가 0인 접미어를 걸러내어 suffixIndex가 언제나 1 이상이므로 if 문 자체가 필요 없다.

compactString에 있는 if문이 모두 필요 없어 보여 제거후 테스트를 하니 모두 통과했다. 불필요한 if문을 제거하여 더 깔끔하게 만든다.

1
2
3
4
5
6
7
private String compactString(String source) {
    return computeCommonPrefix() +
            DELTA_START +
            source.substring(prefixLength, source.length() - suffixLength) +
            DELTA_END +
            computeCommonSuffix();
}

최종 코드

  1
  2
  3
  4
  5
  6
  7
  8
  9
 10
 11
 12
 13
 14
 15
 16
 17
 18
 19
 20
 21
 22
 23
 24
 25
 26
 27
 28
 29
 30
 31
 32
 33
 34
 35
 36
 37
 38
 39
 40
 41
 42
 43
 44
 45
 46
 47
 48
 49
 50
 51
 52
 53
 54
 55
 56
 57
 58
 59
 60
 61
 62
 63
 64
 65
 66
 67
 68
 69
 70
 71
 72
 73
 74
 75
 76
 77
 78
 79
 80
 81
 82
 83
 84
 85
 86
 87
 88
 89
 90
 91
 92
 93
 94
 95
 96
 97
 98
 99
100
101
102
103
104
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 = 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(startingEllipsis())
                .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() - suffixLength;
        int contextEnd = Math.min(contextStart + contextLength, expected.length());
        return expected.substring(contextStart, contextEnd);
    }

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

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

처음 추출했던 메서드 몇 개를 formatCompactedComparison에다 도로 집어넣었다.
shouldNotBeCompacted 조건을 되돌렸다.

코드를 리펙터링 하다보면 원래 했던 변경을 되돌리는 경우가 흔하다.

리펙터링은 코드가 어느 수준에 이를때까지 수많은 시행착오를 반복하는 작업이다.

JUnit 프레임워크 ComparisonCompactor.java 모듈 개선하기