15장
자바 프레임워크 중 가장 유명한 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 모듈을 살펴볼텐데, 다음 코드는 잘 분리된 코드, 적절한 표현력, 단순한 구조를 가진 코드다.
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);
}
}저자는 디팩토링을한 코드를 보여준다.
- 매개변수의 String 변수명을 s1, s2 등으로 사용
- suffix 를 sfx로 줄여 사용
- expected, actual을 cmp1, cmp2로 사용 등
책의 앞장에서 언급했던 안좋은 사용 습관들을 적용하며 얼마나 코드가 보기 힘들어질 수 있는지 보여준다.
JUnit 작성자들이 짠 코드가 디팩터링한 코드보다 얼마나 잘 짜여진 코드인지 보여준다. 하지만, 보이스카우트 규칙(떠날 때에는 처음 왔을 때 보다 더 깨끗한 상태로 남겨놓고 떠나야한다)을 따르기 위해 개선을 해보도록 한다.
멤버 변수 접두어(f) 제거
오늘날의 개발 환경에서는 중복되는 정보인 멤버 변수의 접두어는 필요 없다.
private int ContextLength;
private String Expected;
private String Actual;
private int Prefix;
private int Suffix;
조건문 캡슐화
조건문을 적절한 메서드로 뽑아낸다.
- 변경 전
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();
}서로다른 의미의 변수명
아래 코드의 반환 값은 compact된 문자열이고 this가 붙은 변수들은 입력값들이다 둘의 의미가 다르다. 구별해준다.
String expected = compactString(this.expected);
String actual = compactString(this.actual);.
String compactExpected = compactString(expected);
String compactActual = compactString(actual);부정문보다 긍정문
부정문보다는 긍정문이 인지적으로 이해하기 쉽다. 조건문을 긍정문으로 바꾸고, 로직의 위치를 반전시켜준다.
- 변경 전
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();
}행동을 명확히 묘사하는 함수명
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();
indCommonSuffix();
compactExpected = compactString(expected);
compactActual = compactString(actual);
}또한 compactExpected와 ccompactActual은 멤버 변수로 승격했다.
일관적인 함수 사용방식
새 함수인 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라는 단어를 붙여주도록 변경했다.
시간적인 결합
findCommonSuffix 함수는 시간적인 결합이 존재한다.
findCommonSuffix는 findCommonPrefix가 계산한 prefixIndex를 토대로 동작한다. 그러므로 findCommonPrefix가 먼저 실행되고 난 후 findCommonPrefix가 실행되어야 한다.
다음 두가지 해결방법이 있다.
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 함수가 얼마나 지저분한지도 드러난다. 개선 해준다.
추가 개선
- 개선 후
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)도 마찬가지다.
최종 코드
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 : "");
}
}모듈은 일련의 분석 함수와 일련의 조합 함수로 나뉜다.
전체 함수는 위상적으로 정렬하여 각 함수가 사용된 직후에 정의된다.
분석 함수가 먼저 나오고 조합 함수가 뒤에 이어 나온다.
앞에서 개선했던 몇가지 수정 사항들을 번복하기도 했다. 이런 경우는 흔하다(시행착오는 당연한 것)
결론
보이스카우트 규칙을 잘 지킬 수 있었다. 원래 깨끗하지 못했던 코드라는 말이 아니고 충분히 우수한 모듈이었지만 세상에는 개선이 불필요한 모듈은 없다.
코드를 처음보다 조금 더 깨끗하게 만드는 책임은 우리에게 있다.
느낀점
잘 짜여진 코드라도 충분히 개선의 여지가 있다는 것을 알았다. 단순한 네이밍 뿐만아니라, 함수의 반환에 대한 일관성 시간적인 결합에 대해서도 고려해볼만한 사항이 있다는 것을 깨달았다. 재사용성이 없어보이는 함수라도 조건문과 같은 것들을 함수로 빼내어 그 의미를 파악하기 쉽게 만든다면 충분한 가치가 있다는 것을 다시 한번 떠올릴 수 있었다.
읽으면 읽을수록 코드를 잘 짜고 싶어하는 누군가에게 추천해주고 싶은 책이란 것을 다시한번 느꼈다.
.jpg)
be