CS Clean Code - 냄새와 휴리스틱

아래 목록은 다양한 프로그램을 검토하고 리팩터링하면서 만들었다. 프로그램을 수정할 때마다 나는 "왜?"라고 자문한 다음 그 답을 기록했다. 코드를 읽으면서 나쁜 냄새를 정리하다 보니 목록이 상당히 길어졌다.

 

주석

부적절한 정보

다른 시스템에(소스 코드 관리 시스템, 버그 추적 시스템, 기록 관리 시스템) 저장할 정보를 주석으로 저장하는 것은 적절하지 못하다. 예를 들어 작성자, 변경 이력 같은 정보는 코드만 번잡하게 만든다.

 

쓸모 없는 주석

오래된 주석, 엉뚱한 주석, 잘못된 주석은 더 이상 쓸모가 없다. 쓸모 없는 주석은 아예 달지 않는 편이 가장 좋다. 쓸모 없어진 주석은 가능한 빨리 삭제하는 것이 좋다. 쓸모 없는 주석은 코드와 무관하게 혼자서 따로 놀며 코드를 그릇된 방향으로 이끈다.

 

중복된 주석

코드만으로 충분하데 구구절절 설명하는 주석이 중복된 주석이다. 주석은 코드만으로 다하지 못하는 설명을 부연한다.

 

성의 없는 주석

작성할 가치가 있는 주석은 잘 작성할 가치도 있다. 주석을 달 참이라면 시간을 들여 최대한 멋지게 작성한다.

 

주석 처리된 코드

코드를 읽다가 주석으로 처리된 코드가 줄줄이 나오면 신경이 아주 거슬린다. 얼마나 오래된 코드인지, 중요한 코드인지 아닌지, 알 길이 없다. 그럼에도 아무도 삭제하지 않는다. 누군가에게 필요하거나 다른 사람이 사용할 코드라 생각하기 때문이다. 결국 점차 코드와 멀어지고 그릇된 정보를 제공한다.

 

주석으로 처리된 코드를 발견하면 즉각 지워버려라! 소스 코드 관리 시스템은 기억하니까. 누군가 정말로 필요하다면 이전 버전을 가져오면 된다.

 

환경

여러 단계로 빌드해야 한다

불가해한 명령이나 스크립트를 잇달아 실행해 각 요소를 따로 빌드할 필요가 없어야 한다. 온갖 JAR 파일, XML 파일, 기타 시스템에 필요한 파일을 찾느라 여기저기 뒤적일 필요가 없어야 한다. 한 명령으로 전체를 체크아웃해서 한 명령으로 빌드할 수 있어야 한다.

 

여러 단계로 테스트해야 한다.

모든 단위 테스트는 한 명령으로 돌려야 한다. IDE에서 버튼 하나로 모든 테스트를 돌린다면 가장 이상적이다.

 

함수

너무 많은 인수

함수에서 인수 개수는 작을수록 좋다. 아예 없으면 가장 좋다. 넷 이상은 그 가치가 의심스러우므로 최대한 피한다.

 

출력 인수

일반적으로 우리는 인수를 함수 입력으로 해석한다. 함수에서 뭔가의 상태를 변경해야 한다면 함수가 속한 객체의 상태를 변경한다.

 

public void appendFooter(StringBuffer report) {

}

appendFooter(s);

위의 예시를 보면 함수 선언부를 봤을 때 명확하게 s가 출력 인수인 것을 알 수 있지만, 만약 단독으로 쓰인다면??? 충분히 헷갈릴 여지가 있다.

코틀린의 확장 함수를 사용하면 의미를 확실히 표현할 수 있다.

 

플래그 인수

boolean 인수는 함수가 여러 기능을 수행한다는 명백한 증거다.

 

죽은 함수

주석 처리된 코드와 같은 맥락이다. 아무도 호출하지 않는 함수는 삭제한다.

 

일반

경계를 올바로 처리하지 않는다

코드는 올바로 동작해야 한다. 너무나도 당연한 말이다. 스스로의 직관에 의존하지 마라. 모든 경계 조건을 찾아내고, 모든 경계 조건을 테스트하는 테스트 케이스를 작성하라.

 

안전 절차 무시

체르노빌 원전 사고는 책임자가 안전 절차를 차례로 무시하는 바람에 일어났다.

• serialVersionUID를 직접 제어하면 위험하다.
• 컴파일러 경고를 꺼버리면 빌드가 쉬워질지 모르지만 위험하다.
• 실패한 테스트 케이스를 제껴두면 위험하다.

 

중복

이 책에 나오는 가장 중요한 규칙 중 하나이므로 심각하게 숙고하기 바란다.

• 중복을 발견할 때마다 추상화할 기회로 간주한다.
• 중복된 코드를 하위 루틴이나 다른 클래스로 분리하라.
• switch나 if문은 다형성으로 대체한다.
• 알고리즘이 유사하거나 코드가 서로 다른 중복은 Template Method 패턴을 사용한다.

 

추상화 수준이 올바르지 못하다.

추상화는 저차원 상세 개념에서 고차원 일반 개념을 분리한다. 모든 저차원 개념은 파생 클래스에 넣고, 모든 고차원 개념은 기초 클래스에 넣는다.

 

public interface Stack {
    Object pop();
    void push(Object o);
    double percentFull();
}

percentFull 함수는 추상화 수준이 올바르지 못하다. Stack을 구현하는 방법은 다양하고, 어떤 개념은 타당하지만 어떤 구현은 알 방법이 없다.

크기가 무한인 Stack인 경우 알아낼 방법이 없다.

 

기초 클래스가 파생 클래스에 의존한다.

기초 클래스와 파생 크래스를 나누는 이유는 독립성을 보장하기 위해서다. 그러므로 기초 클래스가 파생 클래스를 사용한다면 문제가 있다는 말이다. 기초 클래스는 파생 클래스를 몰라야 마땅하다.

 

정확하라

코드에서 뭔가를 결정할 때는 정확히 결정한다. 코드에서 모호성과 부정확은 의견차나 게으름의 결과다.

• List로 선언할 수 있는 변수를 ArrayList로 선언하는 행동은 지나친 제약이다.
• 모든 변수를 protected로 선언하는 코드는 무절제하다.
• null을 반환할지도 모른다면 null을 반드시 점검한다.
• 병행 특성으로 인해 동시에 갱신할 가능성이 있다면 적절한 잠금 메커니즘을 구현한다.

주의깊게 생각하지 않을 때 List로 선언할 수 있는 변수를 ArrayList 그대로 선언했던 적이 있다.

 

숨겨진 시간적인 결합

때로는 시간적인 결합이 필요하다. 하지만 시간적인 결합을 숨겨서는 안된다. 함수를 작성할 때 함수 인수를 적절히 배치해 함수가 호출되는 순서를 명백히 드러낸다.

 

public class MoogDiver {
    Gradient gradient;
    List<Spline> splines;
    
    public void dive(String reason) {
        saturateGradient();
        reticulateSplines();
        diveForMoog(reason);
    }
}

위 코드에서 세 함수가 실행되는 순서가 중요하다. 하지만 불행히도 위 코드는 이런 시간적인 결합을 강제하지 않는다. 프로그래머가 실수로 reticulateSplines와 saturateGradient 함수 순서를 바꿀 수 있다.

 

public class MoogDiver {
    Gradient gradient;
    List<Spline> splines;

    public void dive(String reason) {
        Gradient gradient = saturateGradient();
        List<Spline>splines = reticulateSplines(gradient);
        diveForMoog(splines, reason);
    }
}

매개 변수를 통해 시간적인 결합을 노출했다. 함수가 복잡해진다고 불평할지도 모르겠다. 맞는 말이다. 원래 숨겨졌던 시간적인 복잡성을 드러낸 셈이다.

함수가 값을 리턴하도록 수정하거나 확장 함수를 이용하면 인수를 추가하지 않고 해결할 수 있는 경우가 생길 것 같다.

 

경계 조건을 캡슐화하라

경계 조건은 빼먹거나 놓치기 십상이다. 경계 조건은 한 곳에서 별도로 처리한다. 코드 여기저기에 +1, -1을 흩어놓지 않는다. 경계 조건은 놓치기 쉽다.

 

함수는 추상화 수준을 한 단계만 내려가야 한다.

함수 내 모든 문장은 추상화 수준이 동일해야 한다. 그리고 그 추상화 수준은 함수 이름이 의미하는 작업보다 한 단계 낮아야 한다.

 

테스트

버그 주변은 철저히 테스트하라

버그는 서로 모이는 경향이 있다. 한 함수에서 버그를  발견했다면 그 함수를 철저히 테스트하는 편이 좋다.