Portfolio

👀 Junit 들여다보기 1️⃣ 멤버 변수 접두어(f) 제거 2️⃣ 조건문 캡슐화 3️⃣ 서로 다른 의미의 변수명 4️⃣ 부정문보다는 긍정문 5️⃣ 행동을 명확히 묘사하는 함수명 6️⃣ 일괄적인 함수 사용방식 7️⃣ 시간적인 결함 8️⃣ 추가 개선 9️⃣ 최종 코드 🧑🏻‍⚖️ 결론

👀 Junit 들여다보기

Junit은 저자가 많다. 하지만 시작은 켄트 벡과 에릭 감마 두 사람이 비행기를 타고 가다 3시간 동안 Junit 기초를 구현했다고 한다.

이번에 살펴 볼 모듈은 문자열 비교 오류를 파악할 때 유용한 코드인 ComparisonCompactor 모듈을 살펴보면서 코드를 개선해본다.

아래는 ComparisonCompactor 모듈에 대한 테스트 코드다. 커버리지가 100%로 모든 작성자들의 장인 정신을 높이 샀다고 한다.

코드참고


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

아래는 ComparisonCompactor 모듈을 디팩토링한 코드이다.

코드 참고

매개변수의 String 변수명을 s1, s2 등으로 사용

suffix를 sfx로 줄여 사용

expected, actual을 cmp1, cmp2로 사용 등


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

코드는 보이스카우트 규칙에 따라 더 깨끗한 코드로 만들어야 한다.

보이스카우트 규칙 : 체크 아웃때보다 더 좋은 코드를 체크인 한다. 즉, 코드 정리를 거듭할수록 더 좋은 코드가 되어야 한다는 의미

🌳

더 좋은 코드를 위해 아래와 같은 규칙을 한번 적용해보자 !

1️⃣ 멤버 변수 접두어(f) 제거

오늘날의 개발 환경에서는 중복되는 정보인 멤버 변수의 접두어는 필요 없다.


// 변경 전
private int fContextLength;
private String fExpected;
private String fActual;
private int fPrefix;
private int fSuffix;

// 변경 후
private int contextLength;
private String expected;
private String actual;
private int prefix;
private int suffix;

2️⃣ 조건문 캡슐화

조건문을 적절한 메서드로 뽑아낸다


// 변경 전
public String compact(String message) {
				// 여기!
        if (fExpected == null || fActual == null || areStringsEqual()) {
            return Assert.format(message, fExpected, fActual);
        }

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

3️⃣ 서로 다른 의미의 변수명

아래 코드 반환 값은 compact된 문자열이고 this가 붙은 변수들은 입력값들이다 둘의 의미가 다르다.


String expected = compactString(this.expected);
String actual = compactString(this.actual);

// 변경 후
String compactExpected = compactString(expected);
String compactActual = compactString(actual);

4️⃣ 부정문보다는 긍정문

부정문보다는 긍정문이 인지적으로 이해하기 쉽다. 조건문을 긍정문으로 바꾸고, 로직의 위치를 반전시켜 준다


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

// 변경 후
if (canBeCompacted()) {
   		...
   	} else {
  	 	return Assert.format(message, expected, actual);
   	}

private boolean canBeCompacted() {
		return expected != null && actual != null && !areStringEqual();
}

5️⃣ 행동을 명확히 묘사하는 함수명

compact 함수는 ?

오류 점검 부가 단계 숨겨짐 : canBeCompacted가 false면 compact하지 않는다.

형식이 갖춰진 문자열을 반환 : format

따라서 formatCompactedComparison 라는 이름이 더 적합하다.

또한 if문 안의 예상 문자열과 실제 문자열을 압축하는 코드는 compactExpectedAndActual라는 별도의 함수로 빼낸다.

이 함수에서 압축만하고 포맷팅을 하는 일은 전적으로 formatCompactedComparison 함수에게 맡긴다.


...
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와 ccompactActual은 멤버 변수로 승격했다.

6️⃣ 일괄적인 함수 사용방식

새 함수인 compactExpectedAndActual의 위 두 줄은 반환값이 없이 바로 할당하고, compactString은 반환값을 토대로 할당한다. 함수 사용 방식에 대한 일관성을 맞춰준다. (반환형 변경)


// 변경 전
private void compactExpectedAndActual() {
	findCommonPrefix();
  findCommonSuffix();
 	compactExpected = compactString(expected);
  compactActual = compactString(actual);
}

// 변경 후
private void compactExpectedAndActual() {
	prefixIndex = findCommonPrefix();
  suffixIndex = findCommonSuffix();
 	compactExpected = compactString(expected);
  compactActual = compactString(actual);
}

private int findCommonPrefix() {
   int prefixIndex = 0;
   int end = Math.min(expectedIndex.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;
}

멤버 변수 이름도 색인 위치를 나타내기 때문에 Index라는 단어를 붙여주도록 변경함

7️⃣ 시간적인 결함

findCommonSuffix 함수는 시간적인 결합이 존재한다.

findCommonSuffix는 findCommonPrefix가 계산한 prefixIndex를 토대로 동작한다. 그러므로 findCommonPrefix가 먼저 실행되고 난 후 findCommonSufix가 실행되어야 한다.

두가지 해결방법이 있다.

1. 시간 결합을 외부에 노출하기 위해 findCommonSuffix의 매개변수에 prefixIndex를 포함


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) != fActual.charAt(actualSuffix)) {
            break;
        }
    }
    return expected.length() - expectedSuffix;
}

썩 좋은 방법은 아니다. 함수의 호출 순서가 확실히 정해지지만 prefixindex가 필요한 이유를 드러내지 않는다. 다른 프로그래머가 원래대로 돌려놓을지도 모른다.

findCommonPrefixAndSuffix()

다시 아까 반환형을 int로 바꿨던것을 void로 되돌려 놓고 findCommonPrefixAndSuffix라는 함수를 만든다.


// 변경 전
private void compactExpectedAndActual() {
	prefixIndex = findCommonPrefix();
  suffixIndex = findCommonSuffix();
 	compactExpected = compactString(expected);
  compactActual = compactString(actual);
}

// 변경 후
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(expectedIndex.length(), actual.length());
   for (; prefixIndex < end; prefixIndex++) {
       if (expected.charAt(prefixIndex) != actual.charAt(prefixIndex)) {
           break;
       }
   }
}

호출하는 순서가 앞서 코친 코드보다 훨씬 분명해진다.

또한, PrefixAndSuffix 함수가 얼마나 지저분한지도 드러난다. 개선 해준다.

8️⃣ 추가 개선


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

고치고 난 후에는 suffixIndex가 실제로는 접미어 길이라는 사실이 드러나므로 Index가아닌 length라는 단어를 사용한다.

그리고 suffixIndex는 0에서 시작하지 않고 1에서 시작하므로 진정한 길이가 아니다. (computeCommonSuffix에 +1이 곳곳에 등장하는 이유)

suffixIndex = 1 을 suffixLength = 0으로 변경하고 그 외 로직들을 다시 올바르게 재조정한다.

computeCommonSuffix 에서 +1 제거

charmFromEnd에 -1 추가

suffixOverlaps에 <= 사용

if (suffixLength > 0) 은 if (suffixLength >= 0) 으로 바뀌어야한다

하지만 suffixLength가 0인 것은 말이 안된다.
심지어 suffixLength가 0인 상황은 없다. -> if문을 제거할 수 있다.
if (prefixLength > 0)도 마찬가지다.

9️⃣ 최종 코드

코드참고


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 = comapct(expected);
            compactActual = comapct(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); suffixLength++) {
            if (charFromEnd(expected, suffixLength) != charFromEnd(actual, suffixLength)) {
                break;
            }
        }
    }

    private boolean suffixOverlapsPrefix(int suffixLength) {
        return actual.length() = suffixLength <= prefixLength || expected.length() - suffixLength <= prefixLength;
    }

    private void findCommonPrefix() {
        int prefixIndex = 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() {
        prefixIndex > 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 : "");
    }
}

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

앞에서 개선했던 몇가지 수정 사항들을 번복하기도 했다. 이런 경우는 흔하다(시행착오는 당연한 것)

🧑🏻‍⚖️ 결론

보이스카우트 규칙을 잘 지킬 수 있었다.

원래 깨끗하지 못했던 코드라는 뜻은 아니고 충분히 우수한 모듈이었지만 세성에 개선이 불필요한 모듈은 없다.

코드를 처음보다 조금 더 깨끗하게 만드는 책임은 우리에게 있다.

JUnit 들여다보기