2. Code Review

지난 강의에서, 좋은 소프트웨어의 세 가지 특성에는 다음과 같은 3가지가 있었다.

1. Safe from bugs (버그로 부터 안전해야 한다.)
2. Easy to understand (다른 사람도 이해하기 쉬워야 한다.)
3. Ready for change (소프트웨어는 항상 바뀌기 때문에 변화에 준비되어야 한다.)

이 세 가지 특성을 목적으로 하여, 이번 강의에서는 좋은 코드의 일반적인 원칙에 대해서 설명할 것이다. 

1. 코드리뷰란 무엇인가

코드리뷰란 코드의 원작자가 아닌 사람들에 의해서 작성된 소스코드의 체계적이고 세심한 연구를 말한다.

코드리뷰에는 두가지 목적이 있다.

1. 코드를 개선시키는 목적 (버그를 찾고 명확하게 코드를 작성하고, 프로젝트의 표준 스타일과 일관되는지를 체크)
2. 프로그래머를 개선시키는 목적 (새로운 언어 특징이나 프로젝트의 표준과 디자인이 변경된 것을 가르쳐주는 목적)

코드에 대한 표준 스타일은 회사마다 다르다. 얼마나 indent를 해야하는지, curly brace를 어디에 위치 시켜야 하는 지 등 디테일한 부분으로는 회사마다 다르고, 이는 개인의 취향과도 다르지만, 이 강의에서는 위의 3가지 특성에 부합하는 코드를 작성하기 위해서의 몇가지 규칙을 설명한다. 회사마다 표준 스타일은 다르지만 자신이 프로젝트를 진행하고 코드를 작성할때에는 항상 일관된 방식으로 코드를 작성해야지 내 개인 스타일에 맞게끔 기준없이 전부다 바꾸면 협업하는 사람이 굉장히 싫어하기 때문에 항상 프로젝트의 컨벤션을 따르는 것이 중요하고, 일관된 방식으로 코드를 작성하는 것이 중요하다고 한다.

 

제거되어야 하는 나쁜 코드를 "Bad smell"을 가지고 있는 코드라고 하며, "Code hygeine"은 반대되는 것이라고 한다. 아래의 smelly한 코드를 예시로 들겠다.

public static int dayOfYear(int month, int dayOfMonth, int year) {
    if (month == 2) {
        dayOfMonth += 31;
    } else if (month == 3) {
        dayOfMonth += 59;
    } else if (month == 4) {
        dayOfMonth += 90;
    } else if (month == 5) {
        dayOfMonth += 31 + 28 + 31 + 30;
    } else if (month == 6) {
        dayOfMonth += 31 + 28 + 31 + 30 + 31;
    } else if (month == 7) {
        dayOfMonth += 31 + 28 + 31 + 30 + 31 + 30;
    } else if (month == 8) {
        dayOfMonth += 31 + 28 + 31 + 30 + 31 + 30 + 31;
    } else if (month == 9) {
        dayOfMonth += 31 + 28 + 31 + 30 + 31 + 30 + 31 + 31;
    } else if (month == 10) {
        dayOfMonth += 31 + 28 + 31 + 30 + 31 + 30 + 31 + 31 + 30;
    } else if (month == 11) {
        dayOfMonth += 31 + 28 + 31 + 30 + 31 + 30 + 31 + 31 + 30 + 31;
    } else if (month == 12) {
        dayOfMonth += 31 + 28 + 31 + 30 + 31 + 30 + 31 + 31 + 30 + 31 + 31;
    }
    return dayOfMonth;
}

---

2. 좋은 코드를 위한 규칙

1. 반복하지 말것. (Don't Repeat Yourself. DRY)

- 코드를 복사하는 것은 safety에 매우 위험하다. 만약 동일한 코드가 서로 다른 두 곳에 위치한다면, 그 코드에 버그가 있을 경우 maintainer가 한쪽은 코드를 수정하지만 다른 곳의 코드는 수정하지 않을 가능성이 크다. (Safe from bugs, ready to change)
 - 복사-붙혀넣기를 할때마다 등에서 전율이 흘러야 한다. 함부러 복사하여 코드를 반복하지 말아야 한다. 

 

 

2. 필요한 곳에 주석을 달아라 (Comments where needed)

 - 좋은 소프트웨어 개발자는 주석을 분별력있게 작성하는데, 좋은 주석은 더 이해하기 쉽고 bug로부터 더 안전하며, 변화에 준비된 것이다. (Easy to understand)
 - 중요한 주석중 하나는 specification인데, 이는 메소드나 클래스 위에 메소드나 클래스의 행동을 정리해 놓은 것을 말한다. 자바에서는 이를 관습적으로 javadoc 주석을 사용한다. 이는 assumption에 대해서도 정리해 놓았다. (지난 강의) 또한 다른 중요한 주석중 하나는 외부에서 복사해온 코드를 어디에서 가지고 왔는지 출처를 남기는 것이다.

// 1. javadoc specification의 예시
/**
 * Compute the hailstone sequence.
 * See http://en.wikipedia.org/wiki/Collatz_conjecture#Statement_of_the_problem
 * @param n starting number of sequence; requires n > 0.
 * @return the hailstone sequence starting at n and ending with 1.
 *         For example, hailstone(3)=[3,10,5,16,8,4,2,1].
 */
public static List<Integer> hailstoneSequence(int n) {
...
}

// 2. 복사한 코드 출처 밝히기
// read a web page into a string
// see http://stackoverflow.com/questions/4328711/read-url-to-string-in-few-lines-of-java-code
String mitHomepage = new Scanner(new URL("http://www.mit.edu").openStream(), "UTF-8")
                                .useDelimiter("\\A").next();​

이렇게 출처를 남기는 이유는 두 가지가 있는데, (1) 저작권때문에 (2) code가 오래되었을때, 다시 그 출처에 해당하는 링크로 가 업데이트 된 코드로 변경하기 위해서 (Ready to change)

* 주의사항 
불필요한 주석은 작성할 필요가 없다. 하지만 불분명한 코드는 주석으로 명시해줄 필요가 있다.

 

 

3. 빨리 실패해라 (Fail fast)

 - 빨리 실패하라는 말은 가능한 빨리 버그를 발견해야 한다는 것이다. 지난 강의에서 보았듯이, static checking은 dynamic checking보다 빠르고, dynamic checking은 no checking보다 빠르다. 위의 dayOfYear 메소드에서, argument를 넘길때 일-월-연도에 대한 순서는 세계적으로 다르기 때문에 다르게 넘어 갈 수 있다. 따라서 이를 해결하기 위해서 static 에러를 발생시킬수 있도록 수정 가능하다. (Safe from bugs)

 

4. 매직 넘버를 피해라 (Avoid magic numbers)

 - 컴퓨터가 인지할 수 있는 숫자는 0과 1밖에 없다. 소스코드에서 설명 없이 나타나는 상수를 매직넘버라고 한다. 숫자를 설명하는 방법은 주석을 사용 할수도 있지만, 상수를 설명하는 명확한 이름으로 변수명을 설정하는것이 좋다.

public static int dayOfYear(int month, int dayOfMonth, int year) {
    if (month == 2) {
        dayOfMonth += 31;
    } else if (month == 3) {
        dayOfMonth += 59;
        ....
    return dayOfMonth;
}​

위의 코드에서 매직 넘버는 총 4개이다. 2, 31, 3, 59가 무엇을 설명하는지를 모른다. 월을 표기하고 싶을때, 월에 해당하는 2, 3 을 enum으로 만들어 사용한다면, 좋은 방법이 될 것이다. 30, 31, 28과 같은 수는 배열이나 리스트, map과 같은 자료구조를 사용한다면 더욱 가독성이 좋을 것다. 또한 31, 59는 치명적인 매직 넘버의 예시이다. 주석이나 설명이 전혀 없을 뿐만 아니라, 계산을 프로그래머가 직접하여 적은것이다 (28 +31 = 59) 
상수를 절대 직접 계산하여 hard code로 입력하지마라. (Easy to understand)
(1) 프로그래밍 언어가 나보다 계산을 더 잘하며 (2) 유지하는 사람도 어디서 왔는지 더 잘 알 수 있기 때문이다.

 

5. 각 변수당 하나의 목적만 가져라 (One purpose for each variable)

위의 코드 예시에서 dayOfMonth 변수는 완전 다른 값을 계산하기 위해서 재사용 되고 있다. 함수의 리턴값또한 달의 일수가 아니다. 
매개변수와 변수를 재사용하지 마라. 변수는 프로그래밍에서 희귀한 자원이 아니기 때문에 재사용할 필요가 없다. 변수가 특정 값을 의미했다가 갑자기 완전 다른 의미로 변한다면 가독성이 떨어지기 때문이다.
메소드로 넘겨받는 매개변수 또한 수정되지 않아야 한다( ready for change를 위해 중요하다) 따라서 메소드 매개변수에 final 키워드를 사용하는것이 중요하다. static checking도 되기 때문. (Easy to understand)

 

6. 좋은 이름을 사용해라 (Use good names)

좋은 이름은 길고 잘 설명하는 이름이다. 이를 통해서 주석 사용을 막을 수 있다.
temp, tmp, data와 같은 이름을 실제로 사용하는 것은 피해야하며 언어마다의 lexical naming convention을 따라서 작성해야한다. (클래스는 대문자로시작, 메소드와 변수는 소문자로 시작, 상수는 대문자, 카멜케이스로 작성, 패키지는 소문자 .구분 ...)
메소드는 대부분 동사이며 클래스나 변수는 명사이다. 명확하고 짧고 축약되지 않은 단어를 선택햐아한다. (Easy to understand)

 

7. whitespace를 가독성을 위해 사용하라. (Use whitespace for readability)

문장이라 생각하고 너무 길어지면 space를 넣고, 읽기쉽고 확인하기 쉽게 작성해한다.
tab키를 들여쓰기를 위해서 절대 사용해서는 안된다. space 문자를 대신 사용하라. 왜냐하면 툴마다 tab이 space를 차지하는 크기가 다르기 때문에 에디터가 다르면 들여쓰기가 완전히 망가진다. (인텔리제이 코드를 이 블로그에 붙혀넣었을때 이상해 지는 이유가 설명이 된다..) (Easy to understand)

8. 전역 변수를 사용하지 마라 (No global variables)

자바에서는 public static으로 선언된 변수가 전역변수 이다. 일반적으로 전역 변수를 매개변수와 리턴값으로 바꿔라. 아니면 호출하는 메소드 객체 안으로 넣어라. 전역 변수가 아닌 것은 제한된 공간안에서만 바뀌어 지기 때문에 버그를 localize할 수 있다.
사용하면 안되는 이유 : http://web.archive.org/web/20160902115611/http://c2.com/cgi/wiki?GlobalVariablesAreBad
(네임스페이스 오염, 메모리 할당문제, 동시성문제 ... 등등) (Safe from bugs)

9. 결과값을 프린트하지말고 리턴시켜라 (Return results, don't print them)

결과값을 디버깅할때만 console로 출력해야 하기 때문에 예외이고, 나머지 경우는 ready to change에 좋지 않다. console로 결과값을 보내면 다른 context에서 사용해야 할때, 다시 작성되야 하기 때문이다. (Ready for change)