목표: 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] 최종코드...생략
결론
- 코드를 리팩터링 하다보면 원래 했던 변경을 되돌리는 경우가 흔하다. 리팩터링은 코드가 어느 수준에 이를 때까지 수많은 시행착오를 반복하는 작업이기 때문이다.
- 리팩토링에는 정답이 있는 것이 아니라 그 당시의 플랫폼이나 언어, 개발환경에 따라 달라지며, 스스로 만족하는 수준에 이를 때까지 반복되는 작업이다.
