설계(Design)
코드 리뷰에서 다루어야하는 가장 중요한 것은 전체적인 코드의 설계입니다. 코드 변경사항의 다양한 부분들이 잘 설계되어 있는가? 변경사항이 코드 베이스에 또는 라이브러리에 속하는가? 시스템의 나머지 부분과 잘 통합되는가? 지금 이 기능을 추가하기 좋은 시점인가?
기능(Functionality)
코드 변경사항이 개발자의 의도대로 잘 동작하는가? 개발자가 의도한 것이 사용자에게 좋은가? 여기서 사용자는 보통 프로그램 사용자일 수 있고 향후에 이 코드를 사용해야 할 사람일 수 있습니다.
대부분 개발자는 코드 리뷰를 할 때 코드가 정상적으로 동작하도록 변경사항을 충분히 테스트해야 합니다. 그러나 리뷰어로서 당신은 정상 범위를 넘는 사례에 대해 생각하고, 동시성 문제를 찾고, 사용자처럼 생각하도록 하고, 코드를 읽는 것 자체만으로 보이는 버그가 없는지 확인해야 합니다.
특히 사용자에게 직접적인 영향이 있는 UI(User Interface)의 변경이 있을 때는 코드의 변경 사항이 잘 동작하는지 확인하는 것도 중요합니다. 코드를 단순히 읽기만하면 변경점이 어떻게 사용자에게 영향을 끼칠지 파악하기 어렵습니다. 이러한 경우에는 개발자에게 기능 데모를 제공하도록 요청할 수 있습니다.
또한 교착 상태(DeadLock) 또는 경쟁 상태(Race Condition)을 유발할 수 있는 병렬 프로그래밍이 있을 때도 중요합니다. 이러한 종류의 문제는 코드를 실행해도 문제를 찾기 어렵기 때문에 주의 깊게 생각할 필요가 있습니다. 교착 상태나 경쟁 상태 문제가 있는 경우 동시성 모델을 사용하지 않는 것이 오히려 좋을 수 있습니다.
복잡도(Complexity)
코드 변경사항이 필요 이상으로 복잡한가? 모든 수준에서 이 항목을 확인해야 합니다. 라인별로 너무 복잡한가? 함수가 너무 복잡한가? 클래스가 너무 복잡한가? 여기서 “너무 복잡하다” 는 말은 보통 코드를 빠르게 이해할 수 없다는 뜻이며 “개발자가 코드를 호출하거나 수정하려고 할 때 버그가 발생할 가능성이 있다”는 뜻입니다.
특정 유형의 복잡도는 코드를 필요 이상으로 일반화하거나 현재 필요하지 않는 기능을 추가하는 것을 말하는 오버 엔지니어링(over-engineering)입니다. 특히 리뷰어들은 오버 엔지니어링에 대해 경계해야 합니다. 미래에 해결해야 하는 문제가 아니라 지금 해결해야 하는 문제를 해결할 수 있도록 해야 합니다.
테스트(Tests)
변경사항에 적합한 단위, 통합, 종단 테스트를 요청하세요. 일반적으로 코드 변경사항이 긴급상황을 처리하는 경우가 아니라면 서비스 되는 프로덕션 코드와 테스트 코드가 같은 변경사항에 있어야 합니다. 변경 사항의 테스트가 올바르고, 분별력있고, 유용한지 확인하십시오. 테스트를 위한 테스트 코드를 만들지 않기 때문에 반드시 사람이 테스트가 유효한지 확인해야 합니다.
코드가 깨지면 실제로 테스트가 실패하는가? 각 테스트가 간단하고 유용한 결과를 보이는가? 테스트가 적절하게 분리되어 있습니까? 테스트 코드도 유지보수되는 코드임을 기억해야 합니다. 단지 프로덕션 코드가 아닌 테스트 코드라고 불필요한 복잡도를 피해야 합니다.
작명(Naming)
개발자가 모든 것에 대해 좋은 이름을 선택했는가? 좋은 이름은 읽기 어려울 정도로 길지 않고, 그 자체가 무엇이고 무엇을 하는지 충분히 전달할 수 있어야 합니다.
주석(Comments)
개발자가 이해할 수 있는 영어로 명확한 주석을 작성했는가? 모든 주석이 실제로 필요한 것인가? 일반적으로 주석은 코드의 존재 이유를 설명하는데 유용하지만 코드가 무엇을 하고 있는지 설명해서는 안됩니다. 코드의 역할 자체가 명확하게 설명되지 않는 경우에는 코드 자체를 더 간단하게 만들어야 합니다.
예외적으로 정규 표현식이나 복잡한 알고리즘은 주석으로 설명하는 것이 좋을 때도 있지만, 일반적으로는 코드 자체가 포함할 수 없는 정보에 대한 것을 남기는 것이 좋습니다.
스타일(Style)
구글에는 모든 주요 언어 및 대부분의 언어에 대한 스타일 가이드를 가지고 있습니다. 코드 변경사항이 적절하게 스타일 가이드를 따르는지 확인해야 합니다.
스타일 가이드에 없는 스타일 포인트를 향샹시키고 싶다면, 주석에 "Nit:" 접두어를 붙여 코드를 향상시킬 수 있지만 필수가 아닌 것을 알려야 합니다. 코드 변경사항에서 개인의 스타일 선호도를 막아서는 안됩니다.
코드 작성자는 주요 스타일 변경사항을 다른 변경 사항에 포함해서는 안됩니다. 무엇이 변경되는지 보기 어렵고, 코드 머지와 롤백을 어렵게 하며 다른 문제들을 발생시킬 수 있습니다.
예를 들어, 코드 작성자가 전체 파일의 코드를 다시 포맷팅하는 경우, 하나의 코드 변경사항으로 먼저 적용하고 그 이후에 기능 변에 대해서 다른 코드 변경사항으로 적용하도록 해야 합니다. (띄어쓰기, 코드 줄 바꿈 등을 전체적으로 변경한 경우에 별도 커밋으로 적용하도록 한다.)
문서화(Documentation)
개발자가 빌드, 테스트, 릴리즈하는 방법 등을 변경하는 경우 README, g3doc 페이지 그리고 관련 문서들도 업데이트를 해야 합니다. 코드 변경사항이 코드를 삭제하거나 더 이상 사용되지 않는 경우 관련 문서도 삭제할 지 고려해야 합니다. 문서가 없으면 요청하십시오.
모든 줄, 모든 코드(Every Line)
리뷰하도록 할당된 모든 코드 라인을 살펴야합니다. 데이터 파일, 자동 생성 코드, 큰 데이터 구조와 같은 것들은 가볍게 훑어도 되지만 사람이 직접 작성한 클래스, 함수, 코드 블록은 주의깊게 살펴보아야 합니다. 분명하게 어떤 코드는 다른 코드보다 더 세심하게 검토할 필요가 있지만, 적어도 모든 코드가 무엇을 하고 있는지 확실히 이해해야 합니다.
만약 코드를 읽는 것이 너무 어렵고 리뷰의 속도를 늦춘다면, 코드 작성자에게 설명을 요구하고 리뷰하기 전에 그들이 명백하게 설명할 때까지 기다려야 합니다. 구글은 훌륭한 소프트웨어 엔지니어를 고용하며 당신은 그 중 하나입니다. 당신이 코드를 이해하지 못한다면, 다른 개발자도 코드를 이해하지 못할 가능성이 높습니다.
코드를 이해했지만 리뷰를 수행할 자격이 없다고 생각되면, 특히 보안, 동시성, 접근성, 국제화 등과 같은 복잡한 문제에 자격이 있는 리뷰어가 있는지 확인해야 합니다.
문맥(Context)
코드 변경사항을 전반적인 문맥에서 보는 것이 종종 도움이 됩니다. 일반적으로 코드 리뷰 도구들은 변경되는 부분 주위의 몇 줄의 코드에만 표시합니다. 때로는 변경 내용이 실제로 의미가 있는지 확인하기 위해 파일 전체를 보아야 합니다. 예를 들어, 단지 네 개 라인만 수정된 줄 알았으나 전체 파일을 보니 50줄의 함수에 들어있는 것일 수 있습니다.
시스템의 전체 문맥에서 코드 변경사항을 생각하는 것도 유용합니다. 이 코드 변경사항이 시스템의 코드 품질을 개선하나요? 아니면 전체 시스템을 더 복잡하게 만들고 덜 테스트되었습니까? 시스템의 코드 품질을 저하시키는 코드 변경사항을 받아들이면 안됩니다. 대부분 합쳐지는 작은 변화를 통해 복잡해지므로 새로운 변경에서의 작은 복잡도도 막는 것이 중요합니다.
좋은 점(Good Things)
코드 변경사항을 리뷰하다가 좋은 점을 발견하면, 코드 작성자에게 알립니다. 코드 리뷰는 종종 실수에 초점을 맞추지만, 모범 사례에 대한 격려와 감사를 해야 합니다. 멘토링 측면에서 개발자에게 잘못된 것을 말하는 것보다 그들이 무엇을 잘했는지를 말하는 것이 훨씬 더 가치 있습니다.
요약(Summary)
코드 리뷰를 수행할 때 아래 사항을 확인해야 합니다.
- 코드가 잘 설계되어있다.
- 기능이 사용자에게 유용하다.
- UI 변경사항이 합리적이고 보기 좋다.
- 병렬 프로그래밍이 안전하게 수행된다.
- 코드가 필요 이상으로 복잡하지 않다.
- 현재 필요없는 기능을 구현하지 않았다.
- 단위 테스트가 적절하다.
- 테스트가 잘 설계되어 있다.
- 개발자가 모든 것에 명확한 작명을 했다.
- 주석은 명확하고 유용하며, 대부분 무엇이 아닌 이유를 설명한다.
- 적절히 문서화되었다.
- 스타일 가이드를 잘 지켰다.
리뷰어는 요청 받은 모든 코드 라인을 리뷰하고, 문맥을 살피고, 코드 품질을 개선하고 있는지, 코드 작성자가 잘한 점에 대해서 칭찬한다.
