CS Clean Code - JUnit 들여다보기

JUnit

JUnit의 저자는 많다. 하지만 시작은 켄트 벡과 에릭 감마 두 사람이다. 코드는 잘 분리되었고, 표현력이 적절하다. 저자들이 모듈을 아주 좋은 상태로 남겨두었지만 보이스카우트 규칙에 따라 개선해보자.

 

기존 코드

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

 

인코딩을 피하라

private String fExpected;
private String fActual;
private int fPrefix;
private int fSuffix;

----------

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

• 가장 먼저 눈에 거슬리는 부분은 접두어 f다. 오늘날 사용하는 IDE는 기능이 좋기 때문에 변수 이름에 범위를 표시할 필요가 없다.
• 접도우 f는 중복되는 정보다. 그러므로 접두어 f를 모두 제거하자.

 

가능하다면 표준 명명법을 사용하라, 조건을 캡슐화하라

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

• 캡슐화되지 않은 조건문이 보인다. 의도를 명확히 표현하려면 조건문을 메소드로 뽑아내 적절한 이름을 붙인다.
• expected, actual에 this도 눈에 거슬린다. 지역변수에 expected, actual이 있는데 맴버 면수에서 f를 빼버리는 바람에 생긴 결과다. 좀 더 명확하게 네이밍한다.

 

부정 조건은 피하라

public String compact(String message) {
    if (shouldNotCompat()) {
        return Assert.format(message, expecteed, actual);
    }
    findCommonPrefix();
    findCommonSuffix();
    String compatExpected = compactString(expecteed);
    String compatActual = compactString(actual);
    return Assert.format(message, expected, actual);
}

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

• 부정문은 긍정문보다 이해하기 약간 더 어렵다. 그러므로 긍정문으로 만들어 조건문을 반전시킨다.

 

이름으로 부수 효과를 설명하라

public String compact(String message) {
    if (canBeCompacted()) {
        findCommonPrefix();
        findCommonSuffix();
        String compatExpected = compactString(expecteed);
        String compatActual = compactString(actual);
        return Assert.format(message, compatExpected, compatActual);
    } else {
        return Assert.format(message, expecteed, actual);
    }
}

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

• 함수 이름이 이상하다. 단순히 압축된 문자열이 아닌 형식이 갖춰진 문자가 반환된다. 함수 이름을 변경하자
• canBeCompacted가 false이면 압축하지 않는 부수 효과가 있다.
• if문 안에서 압축과 형식을 맞추는 작업 2가지를 진행한다. 이를 분리하자.
• 이 과정에서 compactExpected와 compactActual이 맴버 변수로 승격했다.

함수를 쪼개는 과정에서 변수들이 맴버 변수로 승격할 때 좋은 방법은 없을까?

 

일관성 부족

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

• 함수 사용방식이 일관적이지 못하다. findCommonPrefix()와 findCommonSuffix를 변경해 반환값을 만든다.

 

숨겨진 시작적인 결합

private void compactExpectedAndActual() {
    prefixlndex = findCommonPrefix();
    suffixlndex = 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 >= prefix; actualSuffix--, expectedSuffix--) {
        if (expected.charAt(expectedSuffix) != actual.charAt(actualSuffix)) {
            break;
        }
    }
    return expected.length() - expectedSuffix;
}

• findCommonPrefix()와 findCommonSuffix를 자세히 보면 숨겨진 시간적인 결합이 존재한다.
• findCommonSuffix()는 findCommonPrefix 실행 순서에 영향을 받는다.
• 시간적인 결합을 노출하기 위해 매개변수를 받도록 고치자.

 

일관성을 유지하라

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

• 함수 호출 순서는 정해졌지만 다소 자연스럽지 못하다. findCommonSuffix에서 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;
}

• findCommonPrefix와 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가 실제로 접미어 길이라는 사실을 알 수 있었다. prefixIndex도 마찬가지다
• 길이의 의미로 index보다 length가 더 합당하다. suffixIndex는 0이 아닌 1에서 시작하므로 진정한 길이가 아니다.
• 실제로 computeCommSuffix + 1 이런 코드가 등장하는 이유이다.

 

...

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

...

• charFromEnd를 통해서 경계 조건을 캡슐화했다.
• 'actual.length() - suffixLength <= prefixLength'처럼 <를 <=로 바꾸어 +1, -1 같은 불필요한 코드를 줄였다.

 

코드가 상당히 깔끔해졌다. 모듈은 일련의 분석 함수와 조합 함수로 나뉜다. 코드를 주의 깊게 살펴보면 초반에 내렸던 결정을 일부 번복했다는 사실을 알 수 있다. 예를 들면 shouldNotCompat을 만들었지만, 부정문을 긍정문으로 바꾸는 작업, findCommonSuffix에 매개변수를 추가했다가 제거했던 작업이 있다. 코드를 리팩토링 하다보면 원래 했던 변경을 되돌리는 경우가 흔하다. 리팩토링은 수많은 시행착오를 반복하는 작업이기 때문이다.

14장 점진적인 개선과 비슷하다는 생각이 들었습니다.

 

결론

보이스카우트 규칙을 지키며 모듈을 처음보다 더 깨끗하게 만들었다. 세상에 불필요한 모듈은 없다. 코드를 처음보다 조금 더 깨끗하게 만드는 책임은 우리에게 있다.