cleancode #15, Junit들여다보기

목표: Junit 코드를 분석, 리팩토링하는 과정을 공유

• 보이스카우트 규칙: 소프트웨어 개발에서는 모듈을 체크인할 때, 반드시 체크아웃할 때 보다 아름답게(깨끗하게) 한다는 규칙

The Boy Scout Rule : "Always leave the campground cleaner than you found it."
보이 스카웃 규칙 : 언제나 처음 왔을 때보다 깨끗하게 해놓고 캠프장을 떠날 것.

 

[15-2] 코드 수정

1. prefix 제거

f: 범위 정보

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

 

2. 캡슐화되지 않은 조건문 수정

3. 똑같은 이름의 변수를 여기저기서 사용하지 말자

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 compactExpected = compactString(expected);
	String compactActual = compactString(actual);

    return Assert.format(message, expected, actual);
}

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

 

4. 긍정의 조건문을 만들자

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

 

5. 정확한 이름을 부여하라

위 compact 함수는 항상 압축(compact)을 실행하는 게 아니라 조건에 따라 하고 있으며, formatted된 string을 반환한다.

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

 

6. 함수는 한가지 일만 해야한다 / 함수, 변수 분리

...

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

 

7. 일관성 부족

위에서 3, 4번째 줄이 클래스 변수 세팅하는거고 1, 2번째 줄에서는 함수 안에서 해버림.. 모두 빼버리자..

8. 변수 이름 정확하게(fPrefix -> prefix -> prefixIndex)

private void compactExpectedAndActual() {
    prefixIndex = findCommonPrefix(); // 클래스 변수 세팅; 이름 바꿈
    suffixIndex = findCommonSuffix(); // "
    String compactExpected = compactString(expected);
    String 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() { // prefixIndex 사용
    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;
}

 

9. 숨겨진 시간적인 결합(hidden temporal coupling)

변수 세팅의 순서가 중요하면, 명시적으로 보여줘야한다.

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

private int findCommonSuffix(int prefixIndex) {
    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;
}

 

10. 근데 9번이 좀 억지스러워보이고 왜 순서가 필요한지 이유를 설명하지 못한다. 그래서 하나의 함수로 묶으면 좀 더 안전!

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

이제 저 큰 아이를 리팩토링해아겠다는 생각이 든다.

 

10. 캡슐화(for loop 조건, if문 조건)

11. 올바른 변수명 사용(index -> length)

계속 함수를 고치고보니 index가 사실 진짜 index라기보다 길이를 의미하는 것을 알게되었다.

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

length는 1에서 시작하지 않는다.. 관련해서 쫙 바꿔보자

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에 <=를 사용
• suffixIndex를 suffixLength로

 

12. 죽은 코드는 삭제

있으나마나 한 조건문(항상 참)은 과감히 지워야 한다.

 

[15-5] 최종코드...생략

 

결론

• 코드를 리팩터링 하다보면 원래 했던 변경을 되돌리는 경우가 흔하다. 리팩터링은 코드가 어느 수준에 이를 때까지 수많은 시행착오를 반복하는 작업이기 때문이다.
• 리팩토링에는 정답이 있는 것이 아니라 그 당시의 플랫폼이나 언어, 개발환경에 따라 달라지며, 스스로 만족하는 수준에 이를 때까지 반복되는 작업이다.