코드 리뷰의 규칙

들어가기 전

• 리뷰어: 다른 사람이 작성한 코드를 리뷰하는 사람들
• 리뷰이: 본인이 작성한 코드를 다른 사람들에게 리뷰받는 사람

---

코드 리뷰의 7가지 규칙

1. 왜 개선이 필요한지 이유를 구체적으로 설명하기

코드 개선의 필요성을 느끼고 리뷰를 남긴다면 충분한 이유가 뒷받침되어야 함

주관적이거나 추상적이라면 리뷰이가 혼란을 느낄 수 있음

const data = [
  ['데이터베이스', 'A', 3],
  ['교양영어', 'B+', 1],
  ['철학', 'A', 2]
];

변수명만 봐서는 어떤 의도를 가진 변수인지 파악하기가 힘들고, 데이터가 확장되거나 비슷한 자료구조가 추가될 때 문제가 야기될 수 있음 

따라서 리뷰어는 구체적이고 의도를 가진 변수명으로 변경하고자 함

 

 

안 좋은 리뷰

• data 변수 말고 다른 변수명으로 하세요.
• data 변수 말고 grade로 하세요.

→ 변경에 대한 이유를 설명하지 않고 제안도 애매한 리뷰
리뷰이의 입장에서는 왜 다른 변수명으로 변경을 해야 하는지 알 방법이 없음

 

 

좋은 리뷰

• data라는 이름은 현재의 자료구조가 무엇인지 그 의도를 알기가 어렵네요. 학점 정보를 담고 있는 자료구조 같은데 이와 관련된 변수명을 짓는다면 현재 정의한 자료구조가 무엇인지 그 의도를 쉽게 파악할 수 있을 것 같아요.

→ 개선이 필요한 이유를 구체적으로 설명한 피드백

답을 알려주는 것이 아니라 개선의 필요성만 설명하고 있기 때문에 리뷰이는 스스로 생각하고 학습할 계기가 됨

 

 

2. 답을 알려주기보다는 스스로 고민하고 개선 방법을 선택할 수 있게 하기

"000으로 변경하세요", "000을 쓰세요"와 같이 현재 코드의 개선점을 찾아 답을 알려주는 리뷰는 리뷰이 스스로 고민하는 시간을 줄어들게 함

수동적인 개발자가 될 수 있으므로 스스로 고민하고 개선 방법을 찾는 것이 중요함

const result = [];
arr.forEach(item => {
  if(item % 2 === 0) {
    result.push(item);
  }
});

배열을 순회하며 짝수인 값만 result 배열에 추가하는 코드

리뷰어는 배열 내장 메소드를 통해 코드를 변경하고자 함

 

 

안 좋은 리뷰

• arr.filter(item => item % 2 === 0);으로 사용하세요.

→ filter의 사용 코드를 알려주는 피드백

리뷰이 입장에서는 filter의 동작 방식을 제대로 이해하지 못한 채 수정을 하게 됨

또한 비슷한 코드가 여러 군데 있음에도 불구하고 이곳만 수정할 수도 있음

 

 

좋은 리뷰

• 자바스크립트에는 배열에는 다양한 내장 메서드들이 존재합니다. 그중 filter 메서드를 활용해 보세요. 이 메서드를 활용하면 코드량을 줄일 수 있습니다.

→ 답을 알려주기보단 키워드(filter)를 알려줌으로써 어떤 식으로 검색해야 하는지 방법을 제시하는 피드백

리뷰이 입장에서는 동작 방식을 찾아보고 스스로 학습할 기회가 생기고 비슷한 코드를 리팩토링할 수 있음

 

 

3. 코드를 클린하게 유지하고, 일관되게 구현하도록 안내

const checkNumber = (comNum, myNum) => {
  return myNum.reduce((prev, curr) => {
    if (comNum[curr]) return prev + 1;
    return prev;
  }, 0);
}
function calculateEarningRate(list, totalPrizeMoney, investMoney) {
  const result = [];
  for(let i=0; i < list.length; i += 1) {
    result.push((list[i] / investMoney) * 100);
  }
  return result;
}

1. 함수 선언 방식이 일관되지 않음 (함수 표현식과 함수 선언식)
2. 자료구조 반복문 방식이 일관되지 않음 (reduce와 for문)

리뷰어는 이 두가지 상황에 대해 리뷰하고자 함

 

 

안 좋은 리뷰

• 함수 표현식을 사용하셨군요. reduce의 사용도 잘 활용했네요.

→ 코드 전체를 본 것이 아니라 독립적으로 본 피드백

일관된 코드인지 확인하기 위해서는 코드 전체를 봐야 함

 

 

좋은 리뷰

• 함수를 선언하는 두 가지 방식(함수 표현식, 함수 선언식)을 사용했는데 특별한 이유가 아니라면 함수를 선언하는 방식을 스스로 정하고 그 컨벤션 규칙을 따르도록 해보세요. 일관되게 동작하는 코드는 훨씬 보기 좋고 쉽게 수정할 수 있습니다. 그리고 reduce를 통해서 새로운 자료구조를 만든 건 잘했습니다. 하지만 같은 반복 처리를 하는 과정에서 calculateEarningRate에서 for문을 사용한 건 아쉽네요. reduce와 비슷한 방식의 다른 고차 함수를 찾아서
  써보면 더 일관된 코드를 유지할 수 있을 거 같습니다.

→ 리뷰이가 작성한 코드에서 일관되지 않은 부분을 알려주고 어떤 방식으로 수정하면 좋을지 방법을 제안하는 피드백

 

 

4. 리뷰 과정이 숙제 검사가 아닌 학습과정으로 느낄 수 있게 리뷰하기

class Component {
    constructor(node) {
        this.node = node;
    }

    render(child) {
        this.node.appendChild(child);
    }
}

class Header extends Component {
    constructor(node) {
        super(node);
    }
}

class List extends Component {
    constructor(node) {
        super(node);
    }
}

Header, List 클래스가 Component 클래스를 상속받는 코드

리뷰이는 향후 확장성을 고려하여 미리 클래스를 나눴을 수도 있기 때문에 의도를 파악해야 함

만약 그런 것이 아니라면 아쉬운 부분이 보임

1. 자식 클래스의 contructor에서 아무 일을 하지 않고 부모 클래스 호출만 함
2. Component 클래스에서 선언한 메서드도 사용하지 않음

리뷰어는 이러한 무의미한 상속은 불필요하다는 것을 알려주고자 함

 

 

안 좋은 리뷰

• 클래스 상속은 필요 없습니다.

→ 리뷰이의 의도는 파악하지 않은 채 클래스 상속은 필요 없다고 단정 지음

먼저 왜 이렇게 짰는지 의도를 파악하는 것이 우선

 

 

좋은 리뷰

• Component 클래스를 상속받는 건 좋네요. 그러나 자식 클래스에서 부모 클래스를 호출만 하기 때문에 모든 클래스에서 상속받는 건 오히려 중복 코드 같아 보이기도 하는데 이렇게 작성해 주신 이유가 있을까요?

→ '중복 코드'라는 단어를 사용하여 상속의 불필요한 이유를 설명하고, 리뷰이의 생각은 다를 수 있으므로 의도를 물어봄

리뷰이는 이 피드백을 보고 본인이 왜 이렇게 코드를 작성했는지 되돌아보고 어떤 부분을 수정해야 되는지 고민하고 학습할 수 있는 계기가 됨

 

 

5. 피드백 할 게 없으면 칭찬하기

리뷰할 부분이 없는데 흔적을 남기기 위해 억지로 리뷰를 하지는 말기

깔끔하게 짜인 코드에 다른 디자인 패턴을 권유할 경우 리뷰이 입장에서는 잘 짜인 코드임에도 불구하고 안 좋은 코드라 여기고 잘못된 방향으로 아키텍처가 설계될 수 있음

따라서 리뷰할 부분이 없다면 칭찬 피드백을 주는 것도 한 가지 방법

 

칭찬 피드백 예시

• 코드량이 적당해서 읽기 편하네요.
• 많은 고민이 코드에서 엿보이네요.
• 성능에 아주 유리한 코드라고 생각되네요.
• 전에 코드보다 훨씬 좋아진 거 같네요.
• 예외 처리가 꽤 꼼꼼해서 좋네요.
• 함수, 변수명이 직관적이어서 좋네요.

 

 

6. 작은 커밋 단위만을 보지 말고, 전체 코드의 맥락을 살피기

커밋 단위로 리뷰할 경우 전체적인 흐름 파악이 어려움

작은 단락의 코드만 봤을 때는 깔끔해 보였으나 전체 코드상으로는 어색해 보이는 부분을 발견할 수도 있음

 

 

7. 개발자의 성향을 리뷰하는 것은 지양하기

function getDay(date) {
  if (isWeekend(date)) {
    return weekend;
  } else {
    return weekday;
  }
}

주말인지 파악하여 주말일 경우 weekend, 주말이 아닐 경우 weekday를 반환하는 함수

조건문 안에 추가 연산이나 다른 함수 호출 없이 바로 값을 반환하고 있음

 

 

안 좋은 리뷰

• return isWeekend(date) ? weekend : weekday; 로 작성해 보세요. 코드가 깔끔해집니다.

→ 코드가 깔끔해진다는 이유로 if/else 조건문을 삼항 연산자로 변경해달라는 리뷰

위 코드의 경우 추가 연산이 없으므로 조건문에 대한 가독성은 주관적일 수 있음

누군가는 if/else 조건문이 더 읽기 편할 수 있고, 누군가는 삼항 연산자가 더 읽기 편할 수 있음

추가적인 연산이 없음에도 불구하고 삼항 연산자를 사용해야 한다면 피드백을 남기기 전에 서로 충분한 논의가 필요함

 

 

---

📌 아래 글의 내용을 정리하였습니다.

 

효과적인 코드 리뷰를 위한 리뷰어의 자세