17 냄새와 휴리스틱

• 리팩터링이 필요하다고 생각되는 것들에 대해 정리하였다

주석

C1: 부적절한 정보

• 주석은 코드와 설계에 기술적인 설명을 부연하는 수단이다

C2: 쓸모없는 주석

• 쓸모없어질 주석은 아예 달지 않는 것이 가장 좋고 달려있다면 삭제하는 편이 좋다

C3: 중복된 주석

• 주석은 코드만으로 다하지 못하는 설명만 적는다

C4: 성의없는 주석

• 주석을 달거라면 최대한 간결하고 명료하게 작성한다

C5: 주석 처리된 코드

• 주석 처리된 코드는 즉각 지운다
• 기존의 코드는 소스 코드 관리 시스템이 기억하기 때문에 괜찮다

환경

E1: 여러 단계로 빌드해야 한다

• 빌드는 간단히 한 단계로 끝나야 한다
• 한 명령으로 전체를 체크아웃해서 한 명령으로 빌드를 할 수 있어야 한다

git clone mySystem
cd mySystem
mvn package

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

• 모든 단위 테스트는 한 명령으로 돌려야 한다
• 방법이 빠르고 쉽고 명백해야 한다

함수

F1: 너무 많은 인수

• 함수에서 인수 개수는 작을수록 좋고 아예 없으면 가장 좋다

F2: 출력 인수

• 일반적으로 독자는 인수를 출력이 아닌 입력으로 간주한다

appendFooter(report) -> report.appendFooter()

F3: 플래그 인수

• boolean 인수는 함수가 여러 기능을 수행한다는 명백한 증거
• SRP 위반

render(boolean isSuite) -> renderForSuite(), renderForSingleTest()

F4: 죽은 함수

• 아무도 호출하지 않는 함수는 삭제한다
• 죽은 코드는 낭비다, 과감히 삭제하라
• 소스 코드 관리 시스템이 모두 기억한다

일반

G1: 한 소스 파일에 여러 언어를 사용한다

• JSP -> HTML, 자바, 태그 라이브러리, XML, JavaScript 등
• 소스 파일 하나에 언어 하나만 사용하는 방식이 가장 좋다

G2: 당연한 동작을 구현하지 않는다

• 당연한 동작을 구현하지 않으면 코드를 읽거나 사용하는 사람이 더 이상 함수 이름만으로 함수 기능을 직관적으로 예상하기 어렵다

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

• 코드는 올바로 동작해야 한다
• 그런데 우리는 올바른 동작이 아주 복잡하다는 사실을 자주 간과한다
• 스스로의 직관에 의존하지 말고 모든 경계 조건을 찾아내고 모든 경계 조건을 테스트화하는 테스트 케이스를 작성한다

import static com.google.common.base.Preconditions.checkArgument;
import static com.google.common.base.Preconditions.checkNotNull;

  @Transactional
  public Comment write(Id<Post, Long> postId, Id<User, Long> postWriterId, Id<User, Long> userId, Comment comment) {
    checkArgument(comment.getPostId().equals(postId), "comment.postId must equals postId");
    checkArgument(comment.getUserId().equals(userId), "comment.userId must equals userId");
    checkNotNull(comment, "comment must be provided.");
    
    ...

G4: 안전 절차 무시

• 체르노빌 원전 사고는 책임자가 안전 절차를 차례로 무시하는 바람에 일어났다
• 안전 절차를 무시하면 위험하다
• 실패하는 테스트 케이스를 일단 제껴두고 나중으로 미루는 태도는 신용카드가 공짜 돈이라는 생각만큼 위험하다

G5: 중복

• 소프트웨어 설계를 거론하는 저자라면 거의 모두가 이 규칙을 언급한다

데이비드 토머스, 앤디 헌트: DRY(Don't Repeat Yourself)
켄트 백: "한 번, 단 한 번만(Once, and only once)"
론 제프리스: 이 규칙을 "모든 테스트를 통과한다"는 규칙 다음으로 중요하게 꼽았다.

• 코드에서 중복을 발견할 때마다 추상화할 기회로 간주한다
• 중복된 코드는 하위 루틴이나 다른 클래스로 분리
• 중복은 다형성으로 대체 : TEMPLATE METHOD 패턴이나 STRATEGY 패턴으로 중복을 제거한다

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

• 추상화는 저차원 상세 개념에서 고차원 일반 개념을 분리한다
• 추상화로 개념을 분리할 때는 철저해야 한다
• 모든 저차원 개념은 파생 클래스에 넣고, 모든 고차원 개념은 기초 클래스에 넣는다
  • 예를 들어 세부 구현과 관련한 상수, 변수, 유틸리티 함수는 기초 클래스에 넣으면 안된다
  • 기초 클래스는 구현 정보에 무지해야 마땅하다

import java.util.EmptyStackException;

public interface Stack {
    Object pop() throws EmptyStackException;
    void push(Object o) throws FullException;
    double percentFull();
    ...
} 

• percentFull() 함수는 추상화 수준이 올바르지 못하다
• Stack을 구현하는 방법은 다양한데, 어떤 구현은 '꽉 찬 정도'라는 개념이 타당하다
• 어떤 구현은 알아낼 방법이 전혀 없다
• 따라서 함수는 BoundedStack과 같은 파생 인터페이스에 넣어야 마땅하다

G8: 과도한 정보

• 잘 정의된 모듈은 인터페이스가 아주 작지만 작은 인터페이스로도 많은 동작이 가능하다
• 잘 정의된 인터페이스는 많은 함수를 제공하지 않기 때문에 결합도(coupling)가 낮다
• 자료나 유틸리티 함수, 상수와 임시 변수를 숨기고 메서드나 인스턴스 변수가 넘쳐나는 클래스는 피해야 한다

G9: 죽은 코드

• 죽은 코드: 실행되지 않는 코드
• 죽은 코드는 시간이 지나면 악취를 풍기기 시작한다
• 죽은 코드를 발견하면 적절한 장례식을 치뤄주고 시스템에서 제거하라

G10: 수직 분리

• 변수와 함수는 사용되는 이치에 가깝게 정의한다
• 비공개 함수는 처음으로 호출한 직후에 정의한다

    @Transactional
    fun createFolder(request: Folder.FolderCreateRequest): Folder {
        return when (isParentFolder(request.parentId)) {
            true -> {
                val rootFolder = Folder.dtoToEntity(request)
                folderRepository.save(rootFolder)
            }
            false -> {
                ...
            }
        }
    }

    private fun isParentFolder(parentId: Long) = parentId == 0L
        
    ...

G11: 일관성 부족

• 어떤 개념을 특정 방식으로 구현했다면 유사한 개념도 같은 방식으로 구현한다
• 착실하게 적용한다면 간단한 일관성만으로도 코드를 읽고 수정하기가 대단히 쉬워진다

G12: 잡동사니

• 소스파일은 언제나 깔끔하게 정리한다

G13: 인위적 결합

• 서로 무관한 개념을 인위적으로 결합하지 않는다

G14: 기능욕심

• 마틴 파울러가 말하는 코드 냄새 중 하나다
• 클래스 메소드는 자기 클래스의 변수와 함수에 관심을 가져야지 다른 클래스의 변수와 함수에 관심을 가져서는 안된다

public class HourlyPayCalculator {
    public Money calculateWeeklyPay(HourlyEmployee e) {
        final int tenthRate = e.getTenthRate().getPennies();
        final int tenthsWorked = e.getTenthsWorked();
        final int straightTime = Math.min(400, tenthsWorked);
        final int overTime = Math.max(0, tenthsWorked - straightTime);
        final int straightPay = straightTime * tenthRate;
        final int overTimePay = (int) Math.round(overTime * tenthRate * 1.5);
        return new Money(straightPay + overTimePay);
    }
} 

• 위 calculateWeeklyPay 메소드는 HourlyEmployee 클래스에서 온갖 정보를 가져온다.
• 즉, calculateWeeklyPay 메소드는 HourlyEmployee 클래스의 범위를 욕심낸다.

G15: 선택자 인수

• 인수를 넘겨 동작을 선택하는 대신 새로운 함수를 만드는 편이 좋다

G16: 모호한 의도

• 코드를 짤 때는 의도를 최대한 분명히 밝힌다
  • 행을 바꾸지 않고 표현한 수식, 매직 번호 등등은 저자의 의도를 흐린다

G17: 잘못 지운 책임

• 독자가 자연스럽게 기대할 위치에 코드를 배치한다

G18: 부적절한 static 함수

• Math.max(double a, double b)는 좋은 static 메소드다
• 그러나 아래 함수는 static으로 정의하면 안되는 함수다
• 왜인가?

HourlyPayCalculator.calculatePay(employee, overTimeRate)

• 함수를 재정의할 가능성이 존재하기 때문이다
• 수당을 계산하는 알고리즘이 여러 개일지도 모른다
• 일반적으로 static 함수보다 인스턴스 함수가 더 좋다
• 조금이라도 의심스럽다면 인스턴스 함수로 정의한다.
• 반드시 static 함수로 정의해야겠다면 재정의할 가능성은 없는지 꼼꼼히 따져본다

G19: 서술적 변수

• 프로그램 가독성을 높이는 가장 효과적인 방법 중 하나가 계산을 여러 단계로 나누고 중간 값으로 서술적인 변수 이름을 사용하는 것이다

Matcher match = headerPattern.matcher(line);
if (match.find()) {
    String key = match.group(1);
    String value = match.group(2);
}

G20: 이름과 기능이 일치하는 함수

• 이름만으로 분명하지 않기에 구현을 살피거나 문서를 뒤적여야 한다면
  더 좋은 이름으로 바꾸거나 아니면 더 좋은 이름을 붙이기 쉽도록 기능을 정리해야한다

G21: 알고리즘을 이해하라

• 대다수 괴상한 코드는 사람들이 알고리즘을 충분히 이해하지 않은 채 코드를 구현한 탓이다
• 잠시 멈추고 실제 알고리즘을 고민하는 대신 여기저기 if문과 플래그를 넣어보며 코드를 돌리는 탓이다
• 프로그래밍은 흔히 탐험인데 사실 코드가 '돌아갈' 때까지 이리저리 찔러보고 굴려본다
• '돌아간다'는 사실이 중요한게 아니라 작성자가 알고리즘이 올바르다는 사실을 알아야 한다
• 가장 좋은 방법은 함수를 깔끔하고 명확하게 구성하는 것이다

G22: 논리적 의존성은 물리적으로 드러내라

• 한 모듈이 다른 모률에 의존한다면 물리적인 의존성도 있어야 한다
• 논리적인 의존성만으로는 부족하다
• 의존하는 모듈이 상대 모듈에 대해 뭔가를 가정하면 안 된다
• 의존하는 모든 정보를 명시적으로 요청하는 편이 좋다

G23: if/else 혹은 switch/case 보다는 다형성을 사용하라

• 선택 유형 하나에는 switch 문을 한 번만 사용한다
• 같은 선택을 수행하는 다른 코드에서는 다형성 객체를 생성해 switch 문을 대신한다

G24: 표준 표기법을 따르라

• 팀은 업계 표준에 기반한 구현 표준을 따라야 한다
• 표준을 설명하는 문서는 코드 자체로 충분해야 하며 별도 문서를 만들 필요는 없어야 한다
• 팀이 정한 표준은 팀원 모두가 따라야 한다
• 실제 괄호를 넣는 위치는 중요 하지 않다
• 모두가 동의한 위치에 넣는다는 사실이 중요하다
• 이 사실을 이해할 정도로 팀원들이 성숙해야 한다

G25: 매직 숫자는 명명된 상수로 교체하라

• 매직숫자는 상수로 변경하라
• 어떤 상수는 이해하기 쉬우므로, 코드 자체가 자명하다면, 상수 뒤로 숨길 필요가 없다
• 하단 예제에 TWO (int 2)라는 상수가 반드시 필요할까? 어떤 공식은 그냥숫자를 쓰는 편이 훨씬 좋다

double circumference = radius * Math.PI * 2;

• 매직 숫자라는 용어는 단지 숫자만 의미하지 않는다

• 의미가 분명하지 않은 토큰을 모두 가리킨다

G26: 정확하라

• 검색 결과 중 첫 번째 결과만 유일한 결과로 간주하는 행동은 순진하다
• 코드에서 뭔가를 결정할 때는 정확히 결정한다
• 결정을 내리는 이유, 예외 처리 방법을 분명히 알아야한다
• 호출하는 함수가 null을 반환할지도 모른다면 null을 반드시 점검한다
• 코드에서 모호성과 부정확은 제거해야 마땅하다

G27: 관례보다 구조를 사용하라

• 설계 결정을 강제할 때는 규칙보다 관례를 사용한다
• 명명 관례도 좋지만 구조 자체로 강제하면 더 좋다
• enum 변수가 멋진 switch/case 문보다 추상 메서드가 있는 기초 클래스가 더 좋다

G28: 조건을 캡슐화하라

• 조건의 의도를 분명히 밝히는 함수로 표현하라

// Bad
if (timer.hasExpired() && !timer.isRecurrent())
	
// Good
if (shouldBeDeleted(timer))

G29: 부정 조건은 피하라

• 부정 조건은 긍정 조건보다 이해하기 어렵다
• 가능하면 긍정 조건으로 표현한다

G30: 함수는 한 가지만 해야 한다

• 함수를 짜다보면 한 함수 안에 여러 단락을 이어, 일련의 작업을 수행하고픈 유혹에 빠진다
• 한 가지만 수행하는 좀 더 작은 함수 여렷으로 나눠야 마땅하다

// bad
public void pay() {
  for (Employee e : employees) {
    if (e.isPayday()) {
      Money pay = e.calculatePay();
      e.deliveryPay(pay);
    }
  }
}

// good
public void pay() {
  for (Employee e : employees) {
    payIfNeessary(e);
  }
}

public void payIfNecessary(Employee e) {
  if (e.isPayday()) {
    cacluateAndDeliverPay(e);
  }
}

private void calculateAndDeliveryPay(Employee e) {
  Money pay = e.calculatePay();
  e.deliverPay(pay);
}

G31: 숨겨진 시간적인 결합

• 시간적인 결합을 숨겨서는 안 된다
• 함수를 짤 때는 함수 인수를 적절히 배치해 함수가 호출되는 순서를 명백히 드러낸다

// Bad, 세 함수가 실행되는 순서가 중요하다
public class MoogDriver {
  public void dive(String reason) {
    staturateGradient();
    reticulateSplines();
    diveForMoog(reason);
  }
} 

// Good, 일종의 연결소자를 생성해 시간적인 결합을 노출한다
public class MoogDriver {
  public void dive(String reason) {
    Gradient gradient = staturateGradient();
    List<Spline> splines = reticulateSplines(gradient);
    diveForMoog(splines, reason);
  }
}

• 위에서 인스턴스 변수를 그대로 두었다는 사실에 주목한다
• 해당 클래스의 private 메서드에 필요한 변수일지도 모른다
• 그렇다 치더라도 제자리를 찾은 변수들이 시간적인 결합을 좀 더 명백히 드러낼 것이다

G32: 일관성을 유지하라

• 코드 구조를 잡을 때는 이유를 고민해야한다
• 그리고 그 이유를 코드 구조로 명백히 표현해야한다
• 구조에 일관성이 없어 보인다면 남들이 맘대로 바꿔도 괜찮다고 생각한다
• 시스템 전반에 걸쳐 구조가 일관성이 있다면 남들도 일관성을 따르고 보존한다

G33: 경계 조건을 캡슐화하라

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

// Bad
if (level + 1 < tags.length) {
  parts = new Parse(body, tags, level + 1);
  body = null
}

// Good, 경계 조건 변수로 캡슐화
int nextLevel = level + 1;
if (nexLevel < tags.length) {
  parts = new Parse(body, tags, nextLevel);
  body = null
}

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

• 함수내 모든 문장은 추상화 수준이 동일해야한다
• 그리고 그 추상화 수준은 함수 이름이 의미하는 작업보다 한 단계만 낮아야 한다
• 개념은 아주 간단하지만 인간은 추상화 수준을 뒤섞는 능력이 너무나도 뛰어나다
• 추상화 수준 분리는 리팩터링을 수행하는 가장 중요한 이유 중 하나다

G35: 설정 정보는 최상위 단계에 둬라

• 추상화 최상위 단계에 둬야 할 기본값 상수나 설정 관련 상수를 저차원 함수에 숨기면 안된다
• 대신 고차원 함수에서 저차원 함수를 호출할 때 인수로 넘긴다

G36: 추이적 탐색을 피하라

• 일반적으로 한 모듈은 주변 모듈을 모를수록 좋다
• A가 B를 사용하고 B가 C를 사용한다 하더라도 A가 C를 알아야 할 필요는 없다는 뜻이다
• 이를 디미터의 법칙(Law of Demeter)이라 부른다
• 실용주의 프로그래머들은 부끄럼 타는 코드 작성 이라고도 한다
• 요지는 자신이 직접 사용하는 모듈만 알아야 한다는 뜻이다

자바

J1: 긴 import 목록을 피하고 와일드카드를 사용하라

• 패키지에서 클래스를 둘 이상 사용한다면 와일드카드를 사용해 패키지 전체를 가져오라
• 와일드카드 import 문은 때로 이름 충돌이나 모호성을 초래한다
• 다소 번거롭지만 자주 발생하지 않으므로 여전히 와일드카드 import 문이 명시적인 import 문보다 좋다

J2: 상수는 상속하지 않는다

• 어떤 프로그래머는 상수를 인터페이스에 넣은 다음 그 인터페이스를 상속해 해당 상수를 사용한다

public class HourlyEmployee extends Employee {
  int str = Math.min(tents, TENTS_PER_WEEK); // 이 상수는 어디에서 왔을까?
}

public abstract class Employee implements Payroll {
}

public interface Payroll {
  public static final int TENTS_PER_WEEK = 400; // 이렇게 사용하면 안된다
}

• 상수를 상속 계층 맨 위에 숨겨놨다
• 언어의 범위 규칙을 속이는 행위다
• 대신 static import 를사용하라

J3: 상수 대 Enum

• 자바 5는 enum을 제공한다
• public static final int라는 옛날 기교를 더 이상 사용할 필요가 없다
• int 보다 훨씬 더 유연하고 서술적인 강력한 도구다

이름

N1: 서술적인 이름을 사용하라

• 이름은 성급하게 정하지 않고 서술적인 이름을 신중하게 고른다
• 단순히 ‘듣기 좋은’ 충고가 아니다

• 소프트웨어 가독성의 90%는 이름이 결정 한다

• 신중하게 선택한 이름은 추가 설명을 포함한 코드보다 강력하다

N2: 적절한 추상화 수준에서 이름을 선택하라

• 구현을 드러내는 이름은 피하라
• 작업 대상 클래스나 함수가 위치하는 추상화 수준을 반영하는 이름을 선택하라
• 코드를 살펴볼 때마다 추상화 수준이 너무 낮은 변수 이름을 발견 하리라 발견할 때마다 기회를 잡아 바꿔놓아야 한다
• 안정적인 코드를 만들려면 지속적인 개선과 노력이 필요하다

N3: 가능하다면 표준 명명법을 사용하라

• 기존 명명법을 사용하는 이름은 이해하기 더 쉽다
• DECORATOR 패턴을 활용 한다면 장식하는 클래스 이름에 Decorator 라는 단어를 사용해야 한다
• ToString 과 같이 관례가 존재하는 이름은 관례를 따른다
• 프로젝트에 유효한 의미가 담긴 이름을 많이 사용할수록 독자가 코드를 이해하기 쉬워진다

N4: 명확한 이름

• 함수나 변수의 목적을 명확히 밝히는 이름을 선택한다
• 길더라도 명확하고 서술적으로 지어야 한다

N5: 긴 범위는 긴 이름을 사용하라

• 이름 길이는 범위 길이에 비례해야 한다
• 범위가 작으면 아주 짧은 이름을 사용해도 괜찮다
• 하지만 범위가 길어지면 긴 이름을 사용한다
• 이름 범위가 길수록 이름을 정확하고 길게 짓는다

N6: 인코딩을 피하라

• 이름에 유형 정보나 범위 정보를 넣어서는 안 된다

N7: 이름으로 부수 효과를 설명하라

• 함수, 변수, 클래스가 하는 일을 모두 기술하는 이름을 사용한다
• 이름에 부수 효과를 숨기지 않는다
• 실제로 여러 작업을 수행하는 함수에다 동사 하나만 달랑 사용하면 곤란하다

// Bad
getOos();

// Good, 없으면 새로운 객체를 만드는 메서드
createOrReturnOos();

테스트

T1: 불충분한 테스트

• 테스트 케이스는 몇 개나 만들어야 충분할까?
• 잠재적으로 깨질 만한 부분을 모두 테스트해야 한다
• 테스트 케이스가 확인하지 않는 조건이나 검증하지 않는 계산이 있다면 그 테스트는 불완전하다

T2: 커버리지 도구를 사용하라

• 커버리지 도구는 테스트가 빠뜨리는 공백을 알려준다
• 대다수 IDE는 테스트 커버리지를 시각적으로 표현한다

T3: 사소한 테스트를 건너뛰지 마라

• 사소한 테스트는 짜기 쉽다
• 사소한 테스트가 제공하는 문서적 가치는 구현에 드는 비용을 넘어선다

T4: 무시한 테스트는 모호함을 뜻한다

• 때로는 요구사항이 불분명하기에 프로그램이 돌아가는 방식을 확신하기 어렵다
• 선택 기준은 모호함이 존재하는 테스트 케이스가 컴파일이 가능한지 불가능한지에 달려있다

T5: 경계 조건을 테스트하라

• 경계 조건은 각별히 신경 써서 테스트한다
• 알고리즘의 중앙 조건은 올바로 놓고 경계 조건에서 실수하는 경우가 흔하다

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

• 버그는 서로 모이는 경향이 있다
• 한 함수에서 버그를 발견했다면 그 함수를 철저히 테스트하는 편이 좋다
• 십중팔구 다른 버그도 발견한다

T7: 실패 패턴을 살펴라

• 때로는 테스트 케이스가 실패하는 패턴으로 문제를 진단할 수 있다
• 테스트 케이스를 최대한 꼼꼼히 짜라는 이유도 여기에 있다
• 합리적인 순서로 정렬된 꼼꼼한 테스트 케이스는 실패 패턴을 드러낸다

T8: 테스트 커버리지 패턴을 살펴라

• 통과하는 테스트가 실행하거나 실행하지 않는 코드를 살펴보면 실패하는 테스트 케이스의 실패 원인이 드러난다

T9: 테스트는 빨라야 한다

• 느린 테스트 케이스는 실행하지 않게 된다
• 일정이 촉박하면 느린 테스트 케이스를 제일 먼저 건너뛴다
• 그러므로 테스트 케이스가 빨리 돌아가게 최대한 노력한다

결론

• 이 장에서 소개한 휴리스틱과 냄새 목록이 완전하다 말하기는 어렵다
• 여기서 소개한 목록은 가치 체계를 피력할 뿐이다
• 사실상 가치 체계는 이 책의 주제이자 목표다
• 일련의 규칙만 따른다고 깨끗한 코드가 얻어지지 않는다
• 휴리스틱 목록을 익힌다고 소프트웨어 장인이 되지는 못한다
• 전문가 정신과 장인 정신은 가치에서 나온다
• 그 가치에 기반한 규율과 절제가 필요하다

개인적인 감상

• 주석으로 변경 이력을 아주 간단히 달아놓는 경우도 많이 봤는데 이건 약간의 개인차가 있을 것 같다는 생각이 들었다. 변경이 많이 혹은 자주 되는 경우에는 소스코드로 이력을 다 확인하기가 쉽지 않기 때문에 힌트를 남겨놓는 것은 괜찮지 않을까 싶은데 이런 경우에는 차라리 변경 이력에 대한 문서화를 하는게 낫겠다는 결론에 다다랐다. 복잡한 시스템의 경우 실제로는 어떻게 하고 있는지 궁금해졌다.
• 지금까지 책에서 나온 것들을 정리해놨기 때문에 위에 나오는 휴리스틱에 주의한다면 한결 더 나은 코드를 짤 수 있지 않을까 싶었다.