[Clean Code] 15. Junit 들여다보기 - 클린 코드 정독하기

JUnit 프레임워크

JUnit은 자바 프레임워크 중에서 가장 유명하다. 개념은 단순하며 정의는 정밀하고 구현은 우아하다. JUnit은 저자가 많지만 시작은 켄트 벡과 에릭 감마, 두 사람이 비행기를 타고 가다 JUnit을 만들었다.

 

지금부터 살펴볼 예제는 문자열 비교 오류를 파악할 때 유용한 모듈이다. ComparisonCompactor는 두 문자열을 받아 차이를 반환한다. 예를 들어, ABCDE와 ABXDE를 받으면 <…B[X]D…>를 반환한다.

 

다음은 ComparisonCompactor 모듈의 테스트 코드다.

[15-1] ComparisonCompactorTest.java

import junit.framework.ComparisonCompactor;
import junit.framework.TestCase;

public class ComparisonCompactorTest extends TestCase {
    public void testMessage() {
        String failure = new ComparisonCompactor(0, "b", "c").compact("a");
        assertTrue("a expected:<[b]> but was:<[c]>".equals(failure));
    }

    public void testStartSame() {
        String failure = new ComparisonCompactor(1, "ba", "bc").compact(null);
        assertEquals("expected:<b[a]> but was:<b[c]>", failure);
    }

    public void testEndSame() {
        String failure = new ComparisonCompactor(1, "ab", "cb").compact(null);
        assertEquals("expected:<[a]b> but was:<[c]b>", failure);
    }

    public void testSame() {
        String failure = new ComparisonCompactor(1, "ab", "ab").compact(null);
        assertEquals("expected:<ab> but was:<ab>", failure);
    }

    public void testNoContextStartAndEndSame() {
        String failure = new ComparisonCompactor(0, "abc", "adc").compact(null);
        assertEquals("expected:<...[b]...> but was:<...[d]...>", failure);
    }

    public void testStartAndEndContext() {
        String failure = new ComparisonCompactor(1, "abc", "adc").compact(null);
        assertEquals("expected:<a[b]c> but was:<a[d]c>", failure);
    }

    public void testStartAndEndContextWithEllipses() {
        String failure = new ComparisonCompactor(1, "abcde", "abfde").compact(null);
        assertEquals("expected:<...b[c]d...> but was:<...b[f]d...>", failure);
    }

    public void testComparisonErrorStartSameComplete() {
        String failure = new ComparisonCompactor(2, "ab", "abc").compact(null);
        assertEquals("expected:<ab[]> but was:<ab[c]>", failure);
    }

    public void testComparisonErrorEndSameComplete() {
        String failure = new ComparisonCompactor(0, "bc", "abc").compact(null);
        assertEquals("expected:<[]...> but was:<[a]...>", failure);
    }

    public void testComparisonErrorEndSameCompleteContext() {
        String failure = new ComparisonCompactor(2, "bc", "abc").compact(null);
        assertEquals("expected:<[]bc> but was:<[a]bc>", failure);
    }

    public void testComparisonErrorOverlapingMatches() {
        String failure = new ComparisonCompactor(0, "abc", "abbc").compact(null);
        assertEquals("expected:<...[]...> but was:<...[b]...>", failure);
    }

    public void testComparisonErrorOverlapingMatchesContext() {
        String failure = new ComparisonCompactor(2, "abc", "abbc").compact(null);
        assertEquals("expected:<ab[]c> but was:<ab[b]c>", failure);
    }

    public void testComparisonErrorOverlapingMatches2() {
        String failure = new ComparisonCompactor(0, "abcdde",
                "abcde").compact(null);
        assertEquals("expected:<...[d]...> but was:<...[]...>", failure);
    }

    public void testComparisonErrorOverlapingMatches2Context() {
        String failure =
                new ComparisonCompactor(2, "abcdde", "abcde").compact(null);
        assertEquals("expected:<...cd[d]e> but was:<...cd[]e>", failure);
    }

    public void testComparisonErrorWithActualNull() {
        String failure = new ComparisonCompactor(0, "a", null).compact(null);
        assertEquals("expected:<a> but was:<null>", failure);
    }

    public void testComparisonErrorWithActualNullContext() {
        String failure = new ComparisonCompactor(2, "a", null).compact(null);
        assertEquals("expected:<a> but was:<null>", failure);
    }

    public void testComparisonErrorWithExpectedNull() {
        String failure = new ComparisonCompactor(0, null, "a").compact(null);
        assertEquals("expected:<null> but was:<a>", failure);
    }

    public void testComparisonErrorWithExpectedNullContext() {
        String failure = new ComparisonCompactor(2, null, "a").compact(null);
        assertEquals("expected:<null> but was:<a>", failure);
    }

    public void testBug609972() {
        String failure = new ComparisonCompactor(10, "S&P500", "0").compact(null);
        assertEquals("expected:<[S&P50]0> but was:<[]0>", failure);
    }
}

위 테스트 케이스는 ComparisonCompactor 모듈에 대한 코드 커버리지가 100%가 나왔다. 테스트 케이스가 모든 행, 모든 if 문, 모든 for 문을 실행한다는 의미다.

 

다음은 ComparisonCompactor 모듈이다. 코드는 잘 분리되었고, 표현력이 적절하며, 구조가 단순하다.

[15-2] ComparisonCompactor.java (원본)

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

긴 표현식 몇 개와 이상한 +1 등이 눈에 띈다. 하지만 전반적으로 상당히 훌륭한 모듈이다.

 

ComparisonCompactor 모듈은 저자들이 아주 좋은 상태로 남겨두었지만 보이스카우트 규칙에 따르면 우리는 처음 왔을 때보다 더 깨끗하게 해 놓고 떠나야 한다. 따라서 위 코드를 어떻게 개선하면 좋을지 생각해 보자.

 

멤버 변수 접두어 f 제거

먼저, 멤버 변수 앞에 붙인 접두어 f다. 오늘날 사용하는 개발 환경에서는 변수 이름에 범위를 명시할 필요가 없다. 접두어 f는 중복되는 정보이므로 제거하자.

private int contextLength;
private String expected;
private String actual;
private int prefix;
private int suffix;

 

compact() 조건문 캡슐화

다음은 compact 함수 시작부에 캡슐화되지 않은 조건문이 보인다.

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 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.actual도 눈에 거슬린다. 함수에 이미 expected라는 지역 변수가 있는데, fExpected에서 f를 빼버리는 사람에 생긴 결과다. 함수에서 멤버 변수와 이름이 똑같은 변수는 없어야 한다. 이름은 명확하게 붙인다.

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

 

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

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

 

canBeCompacted() 함수명 변경

함수 이름이 이상하다. 문자열을 압축하는 함수라지만 실제로 canBeCompacted가 false이면 압축하지 않는다. 그러므로 함수에 compact라는 이름을 붙이면 오류 점검이라는 부가 단계가 숨겨진다. 게다가 함수는 단순히 압축된 문자열이 아니라 형식이 갖춰진 문자열을 반환한다.

따라서 새 이름에 인수를 고려하면 가독성이 훨씬 더 좋아진다.

public string formatCompactedComparison(String message) {
    ...
}

 

formatCompactedComparison 함수 분리

if 문 안에서는 예상 문자열과 실제 문자열을 진짜로 압축한다. 이 부분을 빼내 compactExpectedAndActual이라는 메서드로 만든다. 하지만 형식을 맞추는 작업은 formatCompactedComparison에게 전적으로 맡긴다. compactExpectedAndActual은 압축만 수행한다.

 

따라서 함수를 분리하면 다음과 같다.

...
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);
    compactActual = compactString(actual);
}

• compactExpected와 compactActual을 멤버 변수로 추가했다는 사실에 주의한다.
• 새 함수에서 마지막 두 줄은 변수를 반환하지만 첫째 줄과 둘째 줄은 반환값이 없다. 함수 사용방식이 일관적이지 못하다.

 

findCommonPrefix()와 findCommonSuffix() 메서를 변경해 접두어 값과 접미어 값을 반환한다.

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를 주의 깊게 살펴보면 숨겨진 시간적인 결합(hidden temporal coupling)이 존재한다. 다시 말해, findCommonSuffix는 finCommonPrefix가 prefixIndex를 계산한다는 사실에 의존한다.

 

만약, findCommonPrefix와 finCommonSuffix를 잘못된 순서로 호출하면 밤샘 디버깅이라는 고생문이 열린다. 그래서 시간 결합을 외부에 노출하고자 findCommonSuffix를 고쳐 prefixIndex를 인수로 넘긴다.

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가 필요한 이유는 설명하지 못한다.

 

prefixIndex가 필요한 이유가 분명히 드러나지 않으므로 다른 방식을 고안해 보자.

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() {
    int 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에서 가장 먼저 findCommonPrefix를 호출한다. 그러면 두 함수를 호출하는 순서가 앞서 고친 코드보다 훨씬 더 분명해진다.

 

findCommonPrefixAndSuffix 함수가 얼마나 지저분한지도 드러난다. 이제 함수를 정리해 보자.

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”가 동의어다. 비록 그렇다 하더라도, “length”가 더 합당하다.

 

실제로 suffixIndex는 0에서 시작하지 않는다. 1에서 시작하므로 진정한 길이가 아니다. computeCommonSuffix에 +1이 곳곳에 등장하는 이유도 여기에 있다. 이를 고쳐보자.

[15-3] ComparisonCompactor.java (중간 버전)

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을 없애고 charFormEnd에 -1을 추가하고 suffixOverlapsPrefix에 ≤를 사용했다. 그런 다음 suffixIndex를 suffixLength로 바꿨다. 이로써 코드 가독성이 크게 높아졌다.

 

+1을 제거하면 compactString에서 다음 문제를 발견할 수 있다.

if (suffixLength > 0)

[15-3]에서 suffixLength가 1씩 감소했으므로 > 연산자를 ≥ 연산자로 고쳐야 마땅하다. 하지만 ≥ 연산자는 말이 안 된다.

코드를 분석해 보면 if 문은 길이가 0인 접미어를 걸러내 첨부하지 않는다. 원래 코드는 suffixIndex가 언제나 1 이상이었으므로 if 문 자체가 있으나마나였다.

 

compactString에 있는 if 문 둘 다 필요 없어 보인다. 두 문장을 모두 주석으로 처리한 후 테스트를 돌려보면 테스트를 통과한다. 그러므로 불필요한 if 문을 제거하고 compactString 구조를 다듬어 좀 더 깔끔하게 만들자.

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

이제 compactString 함수는 단순히 문자열 조각만 결합한다.

 

좀 더 깔끔하게 정리할 여지는 존재한다. 사소하게 이것저것 손볼 곳이 아직 많지만 일일이 설명하는 대신 최종 코드를 제시한다.

[15-4] ComparisonCompactor.java (최종 버전)

package com.example.japring.junit;

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() {
        int prefixIndex = 0;
        int end = Math.min(expected.length(), actual.length());
        for (; prefixIndex < end; prefixIndex++) {
            if (expected.charAt(prefixIndex) != actual.charAt(prefixIndex)) {
                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 : "";
    }
}

• 코드가 상당히 깔끔하다. 모듈은 일련의 분석 함수와 일련의 조합 함수로 나뉜다.
• 전체 함수는 위상적으로 정렬했으므로 각 함수가 사용된 직후에 정의된다.
• 분석 함수가 먼저 나오고 조합 함수가 그 뒤를 이어서 나온다.
• 코드를 주의 깊게 살펴보면 리팩토링 했던 부분을 원래대로 되돌렸다는 사실을 눈치챌 수 있다.
  • 코드를 리팩토링 하다 보면 원래 코드로 되돌리는 경우는 흔하다.
  • 리팩토링은 코드가 어느 수준에 이를 때까지 수많은 시행착오를 반복하는 작업이다.

 

 

결론

우리는 보이스카우트 규칙을 지켰으며, 모듈은 처음보다 조금 더 깨끗해졌다. 이는 원래 코드가 깨끗하지 못했다는 말은 아니다. 저자들은 우수한 모듈을 만들었다. 하지만 세상에 개선이 불필요한 모듈은 없다. 코드를 처음보다 조금 더 깨끗하게 만드는 책임은 우리 모두에게 있다.

 

 

https://link.coupang.com/a/bfFqVj

클린 코드 + 클린 아키텍처 세트

 

https://link.coupang.com/a/bfFq3g

Clean Code(클린 코드):애자일 소프트웨어 장인 정신

"이 포스팅은 쿠팡 파트너스 활동의 일환으로, 이에 따른 일정액의 수수료를 제공받습니다."