15장. JUnit 들여다보기

공부하는 감자·2024년 2월 18일
클린코드
0

클린코드

목록 보기
15/18

이 장의 주제

세상에 개선이 불필요한 모듈은 없다.

  • 자바 프레임워크 중 가장 유명한 JUnit에서 가져온 코드를 평가한다.
  • 일반적인 프레임워크가 그렇듯 개념은 단순하며 정의는 정밀하고 구현은 우아하다.

JUnit 프레임워크

살펴볼 모듈

ComparisonCompactor 모듈

코드 출처: https://github.com/junit-team/junit4/blob/main/src/main/java/junit/framework/ComparisonCompactor.java

  • 두 문자열을 받아 차이를 반환하는, 영리하게 짜인 코드다.
    • 예를 들어, ABCDE 와 ABXDE 를 받아 <…B[X]D…> 를 반환한다.
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;
    }

    @SuppressWarnings("deprecation")
    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);
    }
}

테스트 코드

코드 출처: https://github.com/junit-team/junit4/blob/main/src/test/java/junit/tests/framework/ComparisonCompactorTest.java

  • ComparisonCompactor를 테스트하는 코드이다.
  • 이 코드를 읽는 편이 모듈을 이해하기에 좋다.
    • 모둘에 필요한 기능이 상세히 드러난다.
  • ComparisonCompactor 모듈에 대한 코드 커버리지 분석을 수행하면 100%가 나온다.
    • 테스트 케이스가 모든 행, 모든 if문, 모든 for문을 실행한다는 의미
package junit.tests.framework;

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 testComparisonErrorOverlappingMatches() {
        String failure = new ComparisonCompactor(0, "abc", "abbc").compact(null);
        assertEquals("expected:<...[]...> but was:<...[b]...>", failure);
    }

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

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

    public void testComparisonErrorOverlappingMatches2Context() {
        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);
    }
}

코드 개선

리팩터링한 이유(제목)은 이해한 내용을 바탕으로 붙여보았습니다.

저자들이 모듈을 아주 좋은 상태로 남겨두었지만, 보이스카우트 규칙에 따르면 우리는 처음 왔을 때보다 더 깨끗하게 해놓고 떠나야 한다.

[N6] 멤버 변수 앞에 붙인 접두어 f

  • 오늘날 사용하는 개발 환경에서는 이처럼 변수 이름에 범위를 명시할 필요가 없다.
  • 접두어 f는 중복되는 정보이므로 모두 제거하자.
private int contextLength;
private String expected;
private String actual;
private int prefix;
private int suffix;

[G28] 캡슐화되지 않은 조건문

  • compact 함수 시작부에 캡슐화되지 않은 조건문이 있다.
  • 의도를 명확히 표현하려면 조건문을 캡슐화해야 한다.
    • 즉, 조건문을 메서드로 뽑아내 적절한 이름을 붙인다.
@SuppressWarnings("deprecation")
public String compact(String message) {

		// 메소드로 조건 추출
    if (shouldNotCompact())
	    return Assert.format(message, expected, actual);

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

// 뽑아낸 메서드
private boolean shouldNotCompact() {
    return expected == null || actual == null || areStringsEqual();
}

[N4] 멤버 변수와 동일한 이름의 지역 변수

  • 멤버 변수에서 접두사 f를 제거하면서, compact 함수에서 사용하는 지역 변수와 이름이 겹쳤다.
    • Assert.format(message, expected, actual);
  • 함수에서 멤버 변수와 이름이 똑같은 변수를 사용하는 이유가 무엇일까?
    • 서로 다른 의미가 아닌가?
    • 이름은 명확하게 붙이도록 수정한다.
@SuppressWarnings("deprecation")
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, compactExpected, compactActual);
}

[G29] 부정문을 긍정문으로 수정

  • 부정문은 긍정문보다 이해하기 약간 더 어렵다.
  • 따라서 compact 함수의 첫 문장 if를 긍정으로 만들어 조건문을 반전한다.
@SuppressWarnings("deprecation")
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();
}

[N7] 기능과 다른 함수 이름

  • compact 함수는 canBeCompacted 가 true면 문자열을 압축하고, false면 압축하지 않는 함수다.
    • 함수에 오류 점검이라는 부가 단계가 숨겨진다.
    • 함수는 단순히 압축된 문자열이 아니라 형식이 갖춰진 문자열을 반환한다.
    • 따라서 실제로는 formatCompatedComparison 이라는 이름이 적합하다.
  • 새 이름에 인수를 고려하면 가독성이 훨씬 좋아진다.
// 함수 명 변경
@SuppressWarnings("deprecation")
public String formatCompatedComparison(String message) {
	...
}

[G30] 함수가 하나의 기능을 수행하도록 수정

  • formatCompatedComparison 함수의 if 문 안에서는 예상 문자열과 실제 문자열을 진짜로 압축한다.
  • 따라서 함수를 분리한다.
    • 형식을 맞추는 작업은 formatCompatedComparison 에게 전적으로 맡긴다.
    • compactExpectedAndActul 은 압축만 수행한다.
    • 지역 변수는 멤버 변수로 승격한다.
...
private String compactExpected;
private String compactActual;
...

@SuppressWarnings("deprecation")
public String formatCompatedComparison(String message) {

    if (canBeCompacted()) {
        // 함수 분리
        compactExpectedAndActul();
        return Assert.format(message, compactExpected, compactActual);

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

// 압축만 수행하는 함수 분리
private void compactExpectedAndActul() {
    findCommonPrefix();
    findCommonSuffix();
    // 지역 변수를 멤버 변수로 변경
    compactExpected = compactString(expected);
    compactActual = compactString(actual);
}

[G11] 일관되지 못한 함수 사용방식

  • 새 함수에서 마지막 두 줄은 변수를 반환하지만 첫째 줄과 둘째 줄은 반환값이 없다.
    • 함수 사용방식이 일관되지 못하다.
  • 그래서 findCommonPrefix() 와 findCommonSuffix() 를 변경해 접두어 값과 접미어 값은 반환한다.
private void compactExpectedAndActul() {
    prefix = findCommonPrefix();
    suffix = findCommonSuffix();
    compactExpected = compactString(expected);
    compactActual = compactString(actual);
}
	
// 접두어 값 반환
private int findCommonPrefix() {
    int prefix = 0;
    int end = Math.min(expected.length(), actual.length());
    for (; prefix < end; prefix++) {
        if (expected.charAt(prefix) != actual.charAt(prefix)) {
            break;
        }
    }
    return prefixIndex;
}

// 접미어 값 반환
private int findCommonSuffix() {
    int expectedSuffix = expected.length() - 1;
    int actualSuffix = actual.length() - 1;
    for (; actualSuffix >= prefix&& expectedSuffix >= prefix; actualSuffix--, expectedSuffix--) {
        if (expected.charAt(expectedSuffix) != actual.charAt(actualSuffix)) {
            break;
        }
    }
    return expected.length() - expectedSuffix;
}

[N1] 멤버 변수 이름을 정확하게 수정

  • 색인 위치를 나타내는 멤버 변수 이름을 좀 더 정확하게 바꾼다.
    • 기존: prefix 와 suffix
private int prefixIndex;
private int suffixIndex;

[G31] 숨겨진 시간적인 결함

  • findCommonSuffix() 는 findCommonPrefix() 가 prefixIndex 를 계산한다는 사실에 의존한다.
    • 만약 두 함수를 잘못된 순서로 호출하면 오류가 발생한다.
    • 숨겨진 시간적인 결합 (hidden temporal coupling)
  • 따라서 prefixIndex 를 인수로 넘겨 시간 결합을 외부에 노출하도록 수정한다.

private void compactExpectedAndActul() {
    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;
}

[G32] 다소 자의적인 인수 전달 방식

  • prefixIndex 를 인수로 전달하는 방식은 다소 자의적이다.
    • 함수 호출 순서는 확실히 정해지지만, prefixIndex 가 필요한 이유는 설명하지 못한다.
  • 따라서, 두 함수를 원상복귀한 뒤 findCommonSuffix 에서 findCommonPrefix 를 호출하도록 수정한다.
    • 반환값 부분 제거: findCommonPrefix , findCommonSuffix
    • findCommonSuffix 의 명칭을 findCommonPrefixAndSuffix() 로 변경
private void compactExpectedAndActul() {
    // 접두어, 접미어 계산
    findCommonPrefixAndSuffix();

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

// 접미어 값 계산
private int findCommonPrefixAndSuffix() {
    // 접두어 값을 선행적으로 계산해야 한다.
    findCommonPrefix();

    int expectedSuffix = expected.length() - 1;
    int actualSuffix = actual.length() - 1;
    for (; actualSuffix >= prefix&& expectedSuffix >= prefix; actualSuffix--, expectedSuffix--) {
        if (expected.charAt(expectedSuffix) != actual.charAt(actualSuffix)) {
            break;
        }
    }
    return expected.length() - expectedSuffix;
}
	
// 접두어 값 계산
private void findCommonPrefix() {
    int prefix = 0;
    int end = Math.min(expected.length(), actual.length());
    for (; prefix < end; prefix++) {
        if (expected.charAt(prefix) != actual.charAt(prefix)) {
            break;
        }
    }
    return prefixIndex;
}

[G30] 함수가 하나의 기능을 수행하도록 수정

  • findCommonPrefixAndSuffix() 함수를 정리한다.
private int findCommonPrefixAndSuffix() {
	findCommonPrefix();

	// 코드를 깔끔하게 정리
	int suffixLength = 1;
	for (; !suffixOverlapsPrefix(suffixLength); actualSuffix--, expectedSuffix--) {
		if (charFromEnd(expected, suffixLength) !=
			charFromEnd(actual, suffixLength)) {
			break;
		}
	}
	return expected.length() - expectedSuffix;
}

// 맨 마지막 문자 반환
private char charFromEnd(String s, int i) {
	return s.charAt(s.length() - i);
}

// for문의 조건문 추출
private boolean suffixOverlapsPrefix(int suffixLength) {
	return actual.length() - suffixLength < prefixIndex ||
		actual.length() - suffixLength < prefixIndex;
}

[G33] 적절하지 못한 이름으로 인한 논리 문제

  • 위에서 코드를 수정하고 나니 suffixLength 가 실제로는 접미어 길이라는 사실이 드러난다.
  • prefixIndex 도 마찬가지로, 이 경우 “index”와 “length”가 동의어다.
    • 그렇다고 하더라도 “length”가 더 합당하다.
  • 실제로 suffixLength 는 1에서 시작하므로 진정한 길이가 아니다.
    • 0에서 시작해야 길이의 의미를 갖는 듯하다.
  • 이러한 이유로 suffixIndex + 1 이 코드 곳곳에 등장한다.
    • suffixIndex + 1 에서 +1 제거
    • charFromEnd 에 -1을 추가하고, suffixOverlapsPrefix 에 <=를 사용함으로써 논리적으로 타당하게 만들어준다.
    • 멤버 변수 suffixIndex 를 suffixLength 로 수정한다.
private int suffixLength;
...
private int findCommonPrefixAndSuffix() {
	findCommonPrefix();

	// Length이므로 0부터 시작
	int suffixLength = 0;

	...
	return expected.length() - expectedSuffix;
}

private char charFromEnd(String s, int i) {
	// suffixLength로 바뀌면서 -1 추가
	return s.charAt(s.length() - i - 1);
}

private boolean suffixOverlapsPrefix(int suffixLength) {
	// suffixLength로 바뀌면서 등호(=) 추가
	return actual.length() - suffixLength <= prefixIndex ||
		actual.length() - suffixLength <= prefixIndex;
}
...
// 남은 함수들에서 (suffixIndex + 1) 을 suffixLength 로 수정

private String compactString(String source) {
	String result = DELTA_START + source.substring(prefixIndex, source.length() - suffixLength) + DELTA_END;
	if (prefixIndex > 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 : "");
}

[G9] 불필요한 if 문 제거

  • suffixLength가 1씩 감소했으므로 compactString() 에서 아래 코드에 등호 >= 가 붙어야 한다.
    • 그런데 >= 연산자는 말이 안 된다. > 연산자가 맞다.
if (suffixLength > 0) {
  • if 문은 길이가 0인 접미어를 걸러내 첨부하지 않도록 한다.
    • 원래 코드는 suffixIndex가 언제나 1 이상이었으므로 if 문 자체가 있으나마나했다.
  • compactString() 에 있는 두 개의 if 문 모두 필요 없어 보인다.
    • 주석 처리한 후 테스트 통과 여부를 확인했다.
    • 테스트를 통과했으므로, 불필요한 if 문을 제거한다.
  • 구조도 조금 다듬었다.
    • if문을 제거하면 반환값이 computeCommonPrefix() + result + computeCommonSuffix() 이 되므로, 한 번에 반환하도록 수정
private String compactString(String source) {
	return computeCommonPrefix() +
		DELTA_START + 
		source.substring(prefixIndex, source.length() - suffixLength) + 
		DELTA_END + 
		computeCommonSuffix();
}

결과

최종 코드는 좀 더 리팩토링한 결과물인데, 이건 서적에서 직접 확인하는 편이 좋을 거 같다.

  • 모듈은 일련의 분석 함수와 일련의 조합 함수로 나뉜다.
  • 전체 함수는 위상적으로 정렬했으므로 각 함수가 사용된 직후에 정의된다.
  • 분석 함수가 먼저 나오고 조합 함수가 그 뒤를 이어서 나온다.

최종 코드에 관하여

최종 코드를 보면, 처음 리팩토링 했을 때 했던 결정 일부를 번복했다는 사실을 알 수 있다.

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

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

Reference

참고 서적

📔 Clean Code

참고 사이트

Git: JUnit4

profile
공부하는 감자
책을 읽거나 강의를 들으며 공부한 내용을 정리합니다. 가끔 개발하는데 있었던 이슈도 올립니다.
이전 포스트

14장. 점진적인 개선

다음 포스트

16장. SerialDate 리팩터링

0개의 댓글