CS Clean Code - SerialDate 리펙토링

CS/Clean Code

CS Clean Code - SerialDate 리펙토링

강태종 2022. 4. 12. 10:01

첫째, 돌려보자

SerialDate 코드에서 데이비드는 엄청난 절제력과 전문가 정신을 보여준다. 의도와 목적을 고려하건대, SerialDate는 분명히 '우수한' 코드다. 그럼에도 여기서는 낱낱이 까발긴다.

SerialDateTests라는 클래스는 단위 테스트 몇 개를 포함한다. 실패하는 테스트 케이스는 없지만, 테스트 케이스를 훑어보면 모든 경우를 점검하지 않는다는 사실이 드러난다. 그래서 나는 클로버를 이용해 테스트 코드를 조사했다. 클로버에 따르면 커버리지는 50% 정도였다.

클래스를 철저히 이해하고 리펙터링하려면 높은 커버리지가 필요했다. 그래서 나는 독자적으로 단위 테스트를 구현했다. SerialDate를 리팩터링하면서 나는 모든 테스트 케이스를 통과하게 코드를 손 볼 작정이다.

둘째, 고쳐보자

448쪽 1행

라이선스 정보, 저작권, 작성자, 변경 이력 등 주석이 나온다.

불필요한 주석 제거

법적인 정보는 필요하므로 라이선스 정보와 저작권은 보존한다.
반면, 소스 코드 제어 도구를 이용하므로 작성자, 변경 이력은 없애도 되겠다.

449쪽 61행

import문은 java.text.*와 java.util.*로 줄여도 된다.

import java.text.DateFormatSymbols;
import java.text.SimpleDateFormat;
import java.util.Calendar;
import java.util.GregorianCalendar;

449쪽 67행

Javadoc 주석이 HTML 태그를 사용한다. 이 주석은 4가지 언어(Java, 영어, Javadoc, HTML)를 사용한다. 다양한 언어를 사용하니 모양을 맞추기 어렵다. 게다가 소스 코드에서 <ul>, <li>를 보고 싶은 사람이 어디 있겠는가? 차라리 <pre>로 감싸는 편이 좋다.

앞뒤 행을 띄워 줄을 잘 맞췄지만 Javadoc을 생성하면 원래 위치를 잃는다.

* <p>
* 요구사항 1 : 최소한 엑셀에서 제공하는 날짜 기능은 제공해야 한다.
* 요구사항 2 : 클래스는 불변이다.
* <p>

450쪽 86행

클래스 이름이 SerialDate인 이유가 무엇일까? Serial 단어가 왜 들어갈까? 해당 증거는 467쪽 830행부터 시작하는 주석에 있다. 클래스 이름이 SerialDate인 이유는 일련번호를 사용해 클래스를 구현했기 때문이다.

/**
 * Returns the serial number for the date, where 1 January 1900 = 2 (this
 * corresponds, almost, to the numbering system used in Microsoft Excel for
 * Windows and Lotus 1-2-3).
 *
 * @return the serial number for the date.
 */

나로서는 두 가지가 꺼림칙하다.

일련번호라는 용어는 날짜보다 제품 식별 번호로 더 적합하다. 그래서 서술적인 이름이 아니라 생각한다.
SerialDate라는 이름은 구현을 암시한다. 하지만 실상은 추상 클래스다. 이름의 추상화 수준이 올바르지 못하다.

내 의견은 그냥 Date가 좋다. 불행히 자바 라이브러리에는 Date라는 클래스가 너무 많다. 여러모로 고민한 끝에 나는 DayDate를 쓰기로 결정했다.

SerialDate 클래스가 MonthConstnats를 상속하는 이유는 무엇일까?

public interface MonthConstants {
    ...
    public static final int JANUARY = 1;
    public static final int FEBRUARY = 2;
    ...
}

MonthConstants를 살펴보면 달을 정의하는 상수 모음에 불과하다. 상속하면 MonthContants 같이 표현할 필요가 없어진다. 옛날 자바 프로그래머들이 많이 쓰던 기교인데 이는 바람직하다고 보기 어렵다.

MonthConstants를 분리하고 enum으로 정의해아 마땅하다. 코드를 모두 고치는데 1시간 정도 걸렸다. 하지만 int에서 enum으로 받기 때문에 isValidMonthCode 메서드를 없앨 수 있고, monthCodeToQuarter 같은 오류 검사 코드도 더 이상 필요하지 않는다.

450페이지 91행

private static final long serialVersionUID = -293716040467423637L;

serialVersionUID 변수는 직렬화를 제어한다. 이 변수 값을 변경하면 이전 소프트웨어 버전에서 직렬화한 클래스를 더 이상 인식하지 못한다. 만약 serialVersionUID를 선언하지 않으면 컴파일러가 자동으로 생성한다.

문서를 읽어보면 모두가 직접 선언하라고 권하지만, 나로서는 자동 제어가 훨씬 더 안전하게 여겨진다. 깜빡하고 serialVersionUID를 변경하지 않아 생기는 괴상한 오류를 디버깅하느리 차라리 Exception을 디버깅하는 편이 나으니까.

450 페이지 97행

해당 주석은 일련번호를 언급한다. 좀 더 의미를 파악할 수 있게 주석을 추가한다.

public static final int EARLIEST_DATE_ORDINAL = 2; // 1 / 1 / 1900
public static final int LATEST_DATE_ORDINAL = 2958465; // 12 / 31 / 9999

452페이지 208행

description 변수는 더 이상 사용하지 않는 듯이 보인다. 그래서 관련 변수와 메서드를 모두 제거했다.

452페이지 213행

기본 생성자도 제거했다. 기본 생성자는 컴파일러가 기본으로 생성한다.

final 키워드

인수와 변수 선언에서 final 키워드를 모두 없앴다. 실질적인 가치는 없으면서 코드만 복잡하게 만든다고 판단했기 때문이다. final 키워드를 제거하겠다는 결정은 일부 기존 관례와 어긋난다. 예를 들어, 로버트 시몬스는 "코드 전체에 final을 사용하라"고 강력히 권장한다.

내가 보기에 final 키워드는 final 상수 등 몇 군데를 제하면 별다른 가치가 없으며 코드를 복잡하게 만든다. 어쩌면 내가 이렇게 느끼는 이유는 내가 작성한 테스트가 final 키워드로 잡아낼 오류를 모두 잡아내기 때문인지도 모르겠다.

final을 사용하는 것이 테스트 코드를 작성하는 것보다 간단하다는 생각이 들고, 기존 코드에서 제거할 만큼 코드를 복잡하게 만드는지 의문이 생긴다. 오히려 final 키워드로 인한 오류를 잡아내기 위해 많은 테스트 코드가 생기고, 관리 비용이 증가하지 않을까 의문이 생긴다.

454페이지 259행

...
s = s.trim();
for (int i = 0;i < weekDayNames.length; i++) {
    if (s.equals(shortWeekdayNames[i])) {
        result = i;
        break;
    }
    if (s.equals(weekDayNames[i])) {
        result = i;
        break;;
    }
}
...

for 루프 안에 같은 if 문이 두 번 나오는데 별로 맘에 들지 않는다. 그래서 || 연산자로 하나의 if로 통일했다.

456페이지 377행

    public static String monthCodeToString(final int month) {
        return monthCodeToString(month, false);
    }
    
    public static String monthCodeToString(final int month, final boolean shortened) {
        ...
        if (shortened) {
            
        } else {
            
        }
        ...
    }

한 메서드가 다른 메서드를 호출하며 플래그를 넘긴다. 일반적으로 메서드에 플래그를 넘기는 것은 바람직하지 못하다. 특히 출력 형식을 선택하는 플래그는 가급적 피하는 편이 좋다.

    public String toString() {
        
    }
    
    public String toShortString() {
        
    }

461페이지 562행

public DayDate addMonths(int months) {
    int thisMonthAsOrdinal = 12 * getYear() + getMonth().index - 1;
    int resultMonthAsOrdinal = thisMonthAsOrdinal + months;
    int resultYear = resultMonthAsOrdinal / 12;
    Month resultMonth = Month.make(resultMonthAsOrdinal % 12 + 1);

    int lastDayOfResultMonth = lastDayOfMonth(resultMonth, resultYear);
    int resultDay = Math.min(getDayOfMonth(), lastDayOfResultMonth);
    return DayDateFactory.makeDate(reusltDay, resultMonth, resultYear);
}

addMonths의 알고리즘이 다소 복잡하므로 임시 변수 설명을 사용해 좀 더 읽기 쉽게 고쳤다. 위 메소드를 바꾸면서 뭔가 꺼림칙했다. DayDate.addMonth(5)라는 표현이 객체를 변경하지 않고 새 인스턴스를 반환한다는 사실을 분명히 드러날까?

DayDate date = DateFactory.makeDate(5, Month.DECEMBER, 1952);
date.addMonths(7); // 날짜를 일주일만큼 뒤로 미룬다.

중대한 문제가 아니라고 여길지도 모르지만, 충분히 오해의 소지가 있다. 코드를 읽는 사람은 분명 date 객체를 변경한다고 생각한다.

DayDate date = oldDate.plusDays(5);

그래서 나는 plus로 이름을 바꿨다. 메서드의 원래 의도를 잘 반영하는 이름이라 생각한다.

date.plusDays(5);

반면, 아래 코드는 단순히 date 객체가 변했다고 받아들이기 어렵다.

결론

주석 개선
- 불필요한 주석 제거
- 주석에 다양한 언어 쓰지 않기
- 일련 번호같은 부분은 주석으로 추가 설명하기
enum 사용, 독자적인 파일로 만들기
정적 변수, 정적 메서드(isLeapYear)를 새로운 파일(DateUtil)로 이동
네이밍 변경(add -> plus)
경계값 개선(Month.JANURARY.toInt())