What to look for in a code review

본 문서는 google code review guide를 보고 요약한 글 입니다.

1. 디자인(Design)

리뷰에서 다루어야 할 가장 중요한 것은 CL의 전체적인 설계입니다.

• CL내에서 다양한 코드들의 상호작용이 의미가 있는가?
• 이 변경사항이 다른 시스템과 잘 통합되는가?
• 이 변경사항이 코드베이스 또는 라이브러리에 속하는가?
• 이제 이 기능을 추가해도 괜찮은가?

2. 기능성(Functionality)

먼저, 이 CL이 코드 작성자의 의도대로 작동하는지, 사용자에게 의도한 것은 무엇인지 확인합니다. 사용자는 일반적으로 end user와 향후 이 코드를 사용해야하는 개발자입니다.

코드 작성자는 코드를 리뷰할 때까지 충분히 CL을 테스트해야 합니다. 동시에 리뷰어는 edge case에 대해 생각하고, 동시성 문제를 찾고, end user처럼 생각하고, 버그가 없는지 확인해야 합니다.

UI 변경과 같이, CL이 동작했을때 end user와의 상호작용에 영향을 주는 경우 리뷰어가 CL의 동작을 확인하는 것이 더 중요해집니다. 이 같은 경우 코드를 읽기만 하는 것으로 해당 CL이 end user에게 미치는 영향을 이해하기 힘듭니다. 이런 경우 코드 작성자에게 CL에 대한 데모를 제공해달라고 할 수 있습니다.

위의 상황과 더불어, 일종의 병렬 프로그래밍(parallel programming) 이 있다면 CL의 동작과 기능에 대해 확인하는 것이 중요해집니다. 병렬프로그래밍은 deadlock과 race conditions을 유발할 수 있기 때문입니다. 이러한 종류의 문제는 코드를 실행하여 문제를 찾아내기가 매우 어렵고, 일반적으로 코드 작성자와 리뷰어 모두가 주의 깊게 생각해야 합니다.

3. 복잡성(Complexity)

각 라인, 함수, 클래스등 모든 수준에서 CL의 복잡성을 점검합니다. “너무 복잡하다” 의 의미는 “코드를 읽는 사람이 빨리 이해할 수 없음” 을 의미합니다. 이는 “다른 개발자가 이 코드를 호출하거나 수정하려고 할 때 버그가 발생할 가능성이 있음” 이라는 의미일 수도 있습니다.

코드 작성자가 필요 이상으로 일반화 하거나, 현재 시스템에 필요하지 않은 기능을 추가한 over-engineering 또한 복잡성을 증가시킵니다. 리뷰어는 특히 over-engineegin에 주의해야 합니다. 코드 작성자가 미래에 해결해야 할 것으로 추측되는 문제가 아닌, 지금 해결해야하는 문제를 해결하도록 장려해야 합니다. 미래의 문제는 그 때의 요구사항과 제품의 실제모습을 보고 해결되어야 합니다.

4. 테스트(Tests)

CL의 변경사항에 대해 적합한 수준의 테스트를 요청합니다. 일반적으로 급한 상황이 아닌 이상(hotfix 등) 코드베이스와 동일한 테스트가 추가되어야 합니다.

테스트가 추가된 경우, CL의 테스트가 정확하고 합리적이며 유용한지 확인합니다. 테스트 자체는 테스트되지 않으며, 사람이 직접 유효한지 다음 항목을 확인해야 합니다.

• 코드가 깨지면(broken) 테스트가 실제로 실패하는가?
• 만약 코드가 변경되면 false positives가 실행되는가? (If the code changes beneath them, will they start producing false positives?)
• 각 테스트는 간단하고 유용한 assertion을 수행하는가?
• 테스트가 다른 테스트 메소드와 적절히 분리되어 있는가?

테스트는 유지보수 대상이기도 합니다. 테스트 코드는 main binary에 속하지 않기때문에, 테스트가 복잡해 지지않아야 합니다.

5. 이름짓기(Naming)

코드 작성자가 좋은 이름을 선택했는지 확인합니다. 좋은 이름은 읽기 어렵지 않고 의도한 것이 무엇인지 완전히 전달하는 충분히 긴 이름 입니다.

6. 주석(Comments)

주석이 다른 개발자가 이해하기 쉬운 영어로 되어있는지, 실제로 필요한 주석인지 확인합니다.

일반적으로 주석은 코드가 존재하는 이유를 설명할 때 유용하며, 일부 코드가 수행하는 작업을 설명해서는 안됩니다. 만약 코드 자체가 명확하지 않다면, 코드를 간단하게 만들어야 합니다. 정규표현식과 같은 몇가지 예외가 있지만, 대부분의 주석은 코드 자체에 포함할 수 없는 정보에 대해 기술해야 합니다.

이 CL이전에 있었던 주석을 보는것도 도움이 될 수 있습니다. 어쩌면 지금 CL에 포함할 수 있는 TODO나 다음 작업에 대한 조언 등이 있을수도 있습니다.

주석은 클래스, 모듈 또는 함수의 문서와는 다르며, 코드의 목적과 사용방법 및 동작 방식을 표현해야 합니다.

7. 스타일(Style)

CL이 스타일 가이드를 적절히 따랐는지 확인합니다.

스타일 가이드에 없는 부분에 대해 코드 품질 향상에 대한 조언을 주려면 “Nit:” 접두어를 붙입니다. 이는 코드 작성자가 코드를 개선할 수는 있지만 필수는 아니라고 받아들일 수 있습니다. 개인 스타일 설정만으로 CL의 승인을 막아서는 안됩니다.

코드 작성자는 주요 스타일 변경에 대해 다른 변경사항과 섞인채로 리뷰를 요청하면 안됩니다. 이는 CL에서 기능이 변경된 내용을 보기 어렵게하고 병합 및 롤백을 복잡하게 만들어 다른 문제를 만들어 낼 수 있습니다. 그렇기 때문에 코드 작성자는 전체 파일에 대한 스타일을 변경하려면, 기능 변경과 스타일 변경에 대한 CL을 분리하여 내보내야 합니다.

8. 문서화(Documentation)

CL에 의해 코드를 빌드, 테스트, 릴리즈 방법이 변경되는 경우 README 등 연관된 문서가 업데이트 되었는지 확인합니다. 문서가 없다면 요청하고, 필요하지않다면 삭제를 요청합니다.

9. 모든 줄(Every Line)

리뷰 요청을 받은 코드의 모든 줄을 확인합니다.

데이터 파일, 만들어진 코드(generated code), 큰 데이터 구조 등은 훑어봐도(scan over) 괜찮습니다. 하지만 사람이 작성한 클래스, 함수, 코드 블록은 내부의 내용이 괜찮을거라 생각하며 훑어보면(가볍게 읽으면) 안됩니다. 분명히 일부 코드는 다른 코드보다 신중하게 읽어야 하지만, 적어도 모든 코드가 수행하는 작업을 이해해야합니다.

코드를 읽기가 너무 어려워 리뷰 속도가 늦어지는 경우, 코드 작성자에게 이를 알리고 코드가 명확해 질 때까지 기다려야합니다. 만약 당신이 코드를 이해하지 못한다면, 다른 개발자들도 코드를 이해하지 못할 가능성이 큽니다. 그렇기 때문에 코드 작성자에게 코드를 명확하게 개선해 달라고 요청해야하며, 이는 미래의 다른 개발자가 이 코드를 이해하도록 도울 수 있게됩니다.

코드를 이해했지만 코드의 일부를 리뷰할 수 없는경우(보안, 동시성, 접근성, 국제화 등) 다른 적절한 리뷰어를 찾아야 합니다.

10. 문맥(Context)

CL을 넓은 맥락에서 바라보는 것이 종종 도움이 됩니다. 일반적으로 코드 리뷰 툴은 변경되는 부분 주위에 몇 줄의 코드만 표시합니다. 때로는 해당 CL이 실제로 의미가 있는지 전체 파일을 살펴봐야합니다. 예를 들어, 해당 CL에서 메소드에 네 줄을 추가하지만, 결과적으로 해당 메소드가 50라인이 되어 refactoring이 필요할수도 있습니다.

시스템 전체의 맥락에서 해당 CL을 생각하는 것도 유용합니다. CL이 시스템의 코드 상태를 개선하는지, 아니면 더 복잡하게 만드는지 등, 여러 각도에서 보아야 합니다. 절대 시스템 코드의 상태를 저하시키는 CL을 허용해서는 안됩니다. 대부분의 시스템은 추가되는 많은 작은 변경사항들을 통해 복잡해 지므로, 새로운 변경사항에서 작은 복잡성을 방지하는 것이 중요합니다.

11. Good Things

CL에서 좋은 점을 발견하면 코드 작성자에게 알려야 합니다. 특히 코드 작성자가 리뷰어의 의견 중 하나를 훌륭한 방식으로 구현했을때는 더욱 그렇습니다.

코드 리뷰를 실수에만 초첨을 맞추지말고, 좋은 부분에 대한 격려와 감사 역시 제공해야 합니다. 멘토링 측면에서, 코드 작성자에게 잘못된 점을 말하는 것 보다 잘한 점을 말하는 것이 더 가치있는 경우가 있습니다.

Summary

• 코드 리뷰시 다음 사항을 확인합니다.

  • 코드가 잘 설계되었는가?
  • 이 기능이 사용자(end user와 다른 개발자)들에게 좋은 기능인가?
  • 모든 UI변경이 합리적이고 보기 좋은가(look good)?
  • 모든 병렬 프로그램이 안전하게 작동하는가?
  • 코드가 필요 이상으로 복잡하지 않은가?
  • 개발자가 미래가 아닌 현재 필요한 것을 구현하였는가(over-engineering 하지 않았는가)?
  • 코드에 적절한 unit test가 있는가?
  • 테스트가 잘 설계되었는가?
  • 코드 작성자가 적절한 이름을 선택하였는가?
  • 주석이 명확하고 유용하며 코드 자체가 아닌 코드의 존재이유를 설명하는가(explain why instead of what)?
  • 코드가 적절히 문서화 되었는가?
  • 코드가 팀의 스타일 가이드를 따르고 있는가?

리뷰 요청을 받은 코드의 모든 줄 을 리뷰하고, 문맥과 상황 을 잘 살펴보고, 코드 상태를 개선 하고, 코드 작성자가 시도한 좋은 일에 대해 칭찬 합니다.