개발자 Code Review 가이드
  -     -   In DevOps

Home

code-review devops pr reviewer

Code Review Developer Guide

목차

이번 포스팅은 2019년 9월 5일에 구글에서 공개한 코드리뷰 가이드를 한글로 번역한 MadPlay님의 게시글과, 개발자 Kenneth Truyers 님의 게시글을 읽고, 실무에서 Code Review 문화를 도입하며 중요하다고 생각되는 개념과 느낀 점들을 요약하여 발췌한 게시글입니다.


Good PR (or Good Change Author)

용어 정리

이 문서 중 일부에서는 구글 내 용어가 사용되며 외부 독자를 위해 명확하게 설명합니다.

  • CL : changelist의 약어로 버전 관리(Version Control)에 제출되었거나 코드 리뷰가 진행 중인 독립된 변경 단위입니다.
  • LGTM : Looks Good to Me의 약어입니다. 위의 코드 리뷰를 승인할 때, 리뷰어가 사용합니다.
  • Nit: : 리뷰어들은 항상 무엇인가 더 나은 방법이 있을 수 있다는 의견을 자유롭게 남길 수 있어야 합니다. 그러나 그것이 그다지 중요하지 않다면 “Nit:”와 같은 접두어를 붙여 코드 작성자가 선택할 수 있도록 해야 합니다.
  • PR : Github의 협업 방식과 관련된 Pull Request를 의미합니다.

PR을 하는 이유는 무엇인가?

올바른 풀 요청을 구성하는 요소를 이해하려면 먼저 풀 요청 프로세스를 사용하는 이유를 정의해야 합니다.

  • PR은 코드 베이스에 대한 정보를 공유하는 좋은 방법입니다. 그들은 코드 소유권을 제한합니다.
  • 코드가 생산되기 전에 추가 게이트를 만듭니다.
  • 코드 품질을 향상시킵니다. 실수를 저지르는 다른 사람이 있기 때문 만은 아닙니다. 누군가가 코드를 검토한다는 것을 알고 있다면 적어도 무의식적으로 더 나은 코드를 작성하기 위해 더 많은 노력을 기울일 것입니다.

좋은 PR 이란 무엇인가?

PR을 검토할 때마다 Code Reviewer는 다음과 같은 특성을 찾습니다.

  • 논리적 커밋. 각 커밋은 하나의 일관된 변경이어야 합니다.
  • 모든 커밋 메시지는 설명 적입니다.
  • 커밋 목록은 관련이 있으며 완전한 이야기를 구현합니다.
  • PR의 작업은 정량화 가능한 가치를 더합니다.
  • 코드는 정의된 표준을 준수합니다.
  • 풀 요청은 가능한 한 작습니다.

아래는 위의 목록에 대한 자세한 설명입니다.

커밋 메시지

좋은 커밋 메시지는 Code Reviewer가 작업하는 데 정보를 얻을 수 있는 좋은 수단입니다. 커밋 메시지는 행해지는 일을 설명하는 것이 아닙니다. 그것은 중요한 정보이지만 이야기의 일부일 뿐입니다. 일반적으로 수행되는 작업은 코드에서 쉽게 확인할 수 있습니다. 해당 코드의 History를 검색하는 검토자가 궁금한 것은 특정 구간이 변경된 이유입니다. 커밋 메시지의 주요 규칙은 다음과 같습니다.

  • 변경된 내용.
  • 공식적인 요구 사항을 참조하여 변경이 필요한 이유.
  • 변화의 직간접적인 결과.

커밋에 포함되지 않아도 되는 것은 다음과 같습니다.

  • 변경 한 사람 (이미 커밋의 일부입니다.)
  • 변경이 이루어진 시점 (이미 커밋의 일부입니다.)
  • 이 코드를 개선하는 방법 (이것은 백 로그에 나와야 합니다.)
  • 코드에 대한 느낌

논리적 커밋

PR을 검토할 때 정보의 절반 만 가지고 있으면 코드 품질을 판단하기가 정말 어렵습니다. 따라서 각 커밋에 하나의 일관된 변경 사항이 포함되어 이론적으로 스스로 검토할 수 있어야 합니다.

관련 커밋

PR을 제출하면 단일 테마와 관련이 있어야 합니다. 상황을 명확하게 알 수 없는 경우가 많으므로 코드 검토는 어려운 작업이 될 수 있습니다. PR이 여러 가지 사항을 수정하거나 구현하려고 하면 더 어려워집니다. 이를 피하기 위해 작업 중인 사용자 스토리(이슈)를 여러 관련 커밋으로 나누고 단일 PR로 제출해야 합니다.

PR은 수량화 가능한 가치를 추가합니다.

이것은 고객 요구 사항을 충족시키는 데 사용될 수 있지만 리팩터링, 성능 개선, 새로운 테스트 등일 수도 있습니다. 요구 사항을 구현하는 경우 이것이 가치를 추가한다는 것을 쉽게 알 수 있습니다. 그러나 리팩터 또는 성능 향상의 경우에는 더 어려울 수 있습니다. 이것들에서 당신은 보통 이것을 미리 논의하고 싶습니다. 그럼에도 불구하고 PR에 추가된 가치가 있다는 것이 PR에 제출되면 상당히 분명해야 하며 결국 이것은 수량화 가능한 가치를 추가합니다.

즉 Good PR을 통해 수치화하기 매우 힘든 개발 성과를 수량화 가능한 가치로써 사용하여, 성과 측정에 도움이 되는 수단으로 사용할 수 있습니다.

코드는 정의된 표준을 준수합니다.

인간 Code Reviewer가 검토하는 것은 코드 스타일 같은 것은 아닙니다. (해당 부분은 정적 분석을 통해 자동화되어야 합니다.) 검토가 필요한 다른 표준이 있습니다. 이것들은 데이터 액세스 방법, 반복되는 패턴, 내부 라이브러리 사용 및 자신의 롤링과 같은 것일 수 있습니다. PR을 제출하기 전에 Change Author가 확인해야 합니다.

멘토링

코드 리뷰는 개발자에게 언어, 프레임워크 또는 일반적인 소프트웨어 설계 원칙에 대해 새로운 것을 가르치는 중요한 기능을 할 수 있습니다. 개발자가 새로운 것을 배울 수 있도록 돕는 의견을 남기는 것은 항상 좋습니다. 지식을 공유하는 것은 시간이 지남에 따라서 코드 품질을 개선하는데 도움이 됩니다. 당신의 의견이 순전히 교육적이라면, 반드시 Nit: 접두어를 붙이거나 코드 작성자가 반드시 수정해야 하는 것이 아님을 밝혀야 합니다.



Good Reviewer

반대로, 좋은 PR을 만든다고 해서, 좋은 검토 프로세스가 있는 것은 아닙니다. 코드를 검토하면서 그 반대 면도 매우 중요합니다. 다음은 도움이 될 수 있는 검토 중인 코드에 대한 질문 및 태도 목록입니다.

  • 코드는 문제를 해결합니까?
  • 출자가 의도 한대로 코드가 작동합니까?
  • 문제를 어떻게 해결했습니까?
  • 유용한 추상화가 있습니까?
  • 악마를 옹호하십시오.
  • 이 코드는 테스트에 포함되어 있습니까?
  • 이 코드에는 문서가 포함되어 있습니까?

이 코드는 문제를 해결합니까?

  • 설계(Design): 코드가 잘 설계되었고 시스템에 적합한가?

이상적으로는 코드 작업을 시작하기 전에 사전 조건이 되어야 하지만 명확하지 않은 경우가 있으며 이것이 가장 좋은 질문입니다. 문제가 해결되지 않으면 나머지는 무의미합니다.

제출자가 의도 한대로 코드가 작동합니까?

  • 기능(Functionality): 코드가 작성자의 의도대로 동작하는가? 사용자에게 적합하게 동작하는가?

목적이 올바른 것으로 확인되면 구현이 실제로 올바른지 확인해야 합니다. 이것은 잠재적인 버그를 발견하려고 시도했을 뿐 아니라 제출자가 의도하지 않은 원치 않는 부작용도 발견할 수 있습니다.

이 문제를 어떻게 해결했습니까?

  • 작명(Naming): 개발자가 변수, 클래스, 메소드 등에 명확한 이름을 선택했는가?
  • 주석(Comments): 주석이 명확하고 유용한가?

이것은 까다로운 것입니다. 때로는 개발자 스타일이 크게 다를 수 있습니다. 이 질문의 목적은 개발자가 당신의 방식으로 강요하도록 하는 것이 아니라, 다른 코딩 스타일을 대조하고 하나가 다른 것보다 선호되는지 평가하는 것입니다.

유용한 추상화가 있습니까?

  • 복잡성(Complexity): 더 간단하게 만들 수 있는가? 나중에 코드를 다른 개발자가 보았을 때 쉽게 이해하고 사용 가능한가?

때로는 문제가 다른 곳에서 이미 해결되었을 수도 있지만 저자는 알지 못합니다. 이것을 지적하고 이것을 사용하는 방법에 대한 아이디어를 제안하는 것이 좋습니다. 경우에 따라 기존 코드를 단순히 재사용하는 경우도 있고 의존하는 새로운 추상 개념을 도입해야 하는 경우도 있습니다.

이 코드는 테스트에 포함되어 있습니까?

  • 테스트(Tests): 잘 설계된 자동 테스트가 있는가?

테스트는 회귀 문제뿐만 아니라 문서화에도 중요합니다. 코드가 매우 알고리즘 적이라면 자동화된 단위 테스트가 필요할 수 있습니다 (힌트 : 구현 중인 요구 사항 목록을 제공하므로 검토를 시작하기에 좋은 곳입니다). 반대로, 주로 코드를 조정하는 경우 단위 테스트가 추가되는 이유와 실제로 필요한지에 대해 의문하십시오.

이 코드에는 문서가 포함되어 있습니까?

  • 문서화(Documentation): 개발자가 관련 문서도 업데이트했는가?

문서는 코드 소유권을 제한할 수 있는 가장 정확한 수단입니다. 복잡한 구현일수록 문서화가 포함되어 있어야 합니다.

악마를 옹호하십시오.

“완벽한” 코드란 것은 없으며 더 나은 코드만 있습니다. 실수, 오류, 관습에 대한 위반 등을 잡으려고 노력하십시오. 다른 개발자보다 우수성을 입증하지 않고 코드를 개선하는 것이 목표입니다. 모든 검토 의견은 코드에 관한 것이어야 합니다. 개발자가 아닌 검토 중인 코드입니다.



Code Review의 표준 및 지침 설정 (실무에서 적용하려 할 때..)

좋은 PR을 제출하고 좋은 리뷰를 하는 것 외에도 PR의 거부 시점과 병합될 수 있는 시점을 명확하게 이해하는 것이 퍼즐의 또 다른 부분입니다. 다음은 내가 갖추어야 할 표준 중 일부입니다. 이러한 상황은 상황에 따라 다를 수 있지만 매개 변수에 관계없이 모든 개발자 간에 명확하게 이해하는 것이 좋습니다.

Code Review 거절 사유

어떤 것들은 거부의 분명한 이유 일 수 있습니다. 이것들을 열거하고 모두에게 동의하는 것이 중요합니다. 초기 거부는 Code Reviewer와 Change Author 측 모두에서 많은 시간을 절약할 수 있기 때문입니다. 사소한 문제에 대한 토론을 피합니다.

직선 거부로 이어질 수 있는 몇 가지 사항 :

  • 실패한 테스트 (CI 서버에서 이를 시행하는 것이 더 낫습니다)
  • 스타일 가이드라인을 따르지 않음 (자동 도구를 통해 시행하는 것이 더 낫습니다)
  • 좋은 PR의 규칙에 대한 위반. 이것은 설명이 아닌 커밋 메시지, 매우 큰 PR, 하나의 PR에 여러 층을 의미할 수 있습니다.

갈등 해결

결국 두 개발자가 동의하지 않는 상황에 처하게 됩니다. 그것은 완벽하게 훌륭하고 실제로 좋은 팀 역학의 신호입니다. 그러나 좋지 않은 것은 선임, 관리자 권한 등의 이유로 인해 다른 개발자 또는 다른 개발자를 무시하는 것에 대해 끝없는 토론을 하고 있습니다.

이러한 상황이 발생하지 않도록 하고 충돌 해결 전략을 세우는 것이 좋습니다. 효과적인 방법 중 하나는 항상 최소한 두 명의 코드 검토자가 필요합니다. 그러한 경우에는 단순히 다수결이 중요합니다.

리드 타임 검토 (왜 코드 리뷰를 빠르게 해야 하나?)

사람들이 PR을 건너 뛰는 것을 본 가장 큰 이유 중 하나는 PR을 차단하기 때문입니다. 제품 팀이 배포를 추진할 때 PR이 검토 코드에 며칠 동안 남아있을 때 도움이 되지 않습니다. 이상적으로는 1 시간 이내에 PR을 검토해야 합니다. PR이이 게시물에 언급된 관행을 따르고 있다면 이것이 가능할 것입니다. 고장 난 곳은 대규모 PR에 있습니다. 따라서 PR을 최대한 작게 유지하고 검토가 신속하게 실행되도록 하는 것이 매우 중요합니다.

만약 당신이 너무 바빠서 리뷰하기 어렵다면, 언제쯤 리뷰를 시작할 것인지 또는 다른 리뷰어를 제안하거나 초기에 광범위한 의견을 제공할 수 있습니다.

만약 코드 리뷰가 느리면 다음과 같은 몇 가지 일이 발생합니다.

  • 팀 전체의 속도가 감소합니다. 리뷰를 빠르게 해주지 않은 팀원은 자기 일은 끝냈겠지만, 코드 변경사항은 반영되지 못하고 리뷰를 기다림에 따라 새로운 기능 추가나 버그 수정은 며칠, 몇 주 또는 몇 달씩 지연됩니다.
  • 개발자들이 코드 리뷰 프로세스에 항의하기 시작합니다. 리뷰어가 며칠에 한 번씩만 의견을 주면서 매번 큰 수정사항을 요구하면, 개발자에게 좌절감을 주거나 리뷰가 너무 까다롭다고 생각할 수 있습니다. 종종 이것은 리뷰어가 얼마나 “엄격한” 지에 대한 불평으로 표현됩니다. 개발자가 코드를 수정할 때마다 리뷰어가 의견을 준다면 불평사항은 사라지는 경향이 있습니다. 실제로 리뷰 속도를 빠르게 하면 코드 리뷰 프로세스에 대한 불평은 해결됩니다.
  • 코드 품질에 영향을 줄 수 있습니다. 리뷰가 느리면 개발자가 할 수 있는 것보다 낮은 품질의 코드가 작성될 수 있고 코드 정리, 리팩토링 및 기존 변경사항에 대한 추가적인 개선 의욕을 저해합니다.

나중에 정리할게요

개발자들은 일반적으로 일을 빨리 끝내고 싶어 합니다. 따라서 코드 변경사항을 반영할 때 사소한 내용에 대한 리뷰를 하고 싶어 하지 않습니다. 그래서 나중에 정리한다고 하는데, 당신은 지금 수정되도록 해야 합니다. 몇몇 개발자들은 후속 커밋(변경사항)을 반영하여 이런 문제들을 해결하기도 하지만 경험에 따르면 개발자가 기존 변경사항을 작성한 후 시간이 지날수록 이러한 정리 작업은 덜 일어납니다. 사실, 개발자가 현재의 코드 변경사항을 처리한 후 정리를 하지 않는 한, 정리 작업은 절대 일어나지 않습니다. 개발자가 무책임한 것이 아니라 해야 할 일이 많고, 다른 작업으로 인해 잊혀지기 때문입니다. 따라서 코드 리뷰가 완료되기 전에 지금 코드 정리를 해야 한다고 주장하는 것이 가장 좋습니다. “나중에 정리하도록” 하는 것은 코드의 품질을 떨어뜨리는 일반적인 원인이 됩니다.

엄격한 리뷰에 대한 일반적인 불만

유연한 코드 리뷰를 중단하고 엄격한 리뷰를 적용하면, 일부 개발자들은 매우 크게 불만을 가질 수 있습니다. 코드 리뷰 속도를 높이 일반적으로 이러한 불만은 사라집니다.

이러한 불만이 사라지는 데 몇 개월이 걸릴 수 있지만, 결국 좋은 코드를 만드는데 도움이 된다는 것을 알게 되며 때로는 가장 큰 불만을 가졌던 사람들이 엄격한 코드 리뷰를 더 지지하기도 합니다.



해당 게시글은 기본적인 Git의 협업 방식이나, 정적 분석에 대해 어느정도 알고 있다고 가정합니다. 혹시 해당 부분을 더 알고 싶으시다면

  • Key Word : 정적 분석, 정적 분석 자동화
  • Key Word : Git Workflow, Pull Request

로 검색하시거나,