google code review(2) - 구체적 방식

Navigating a CL in Review

• CL 에 대한 설명이 잘 되어있는지 가장 먼저 확인하자.

• step 1: 변화의 broad view를 보자.

  • 애초에 이러한 변경이 발생하지 말았어야 하는 경우에는, 변경이 발생하지 않아야 하는 이유를 즉시 설명하여 author 에게 전달하라.
  • 이와 같은 변경을 거부할 때, 개발자에게 대신 수행했어야 할 작업을 제안하는 것도 좋은 방법입니다.

• step 2: CL의 main part를 시험하자.

  • 주요 부분인 파일을 찾고, 가장 먼저 보자.
  • CL이 너무 커서 어떤 부분이 주요 부품인지 파악할 수 없는 경우,
  • 개발자에게 먼저 무엇을 살펴야 하는지 물어보거나, CL을 여러 CL로 분할하도록 요청하십시오.

• step 3: 적절한 순서로 CL의 나머지 부분을 살펴보라.

  • 일반적으로 주요 파일을 살펴본 후에는 코드 검토 도구가 제공하는 순서대로 각 파일을 살펴보는 것이 가장 간단합니다.
  • 때로는 메인 코드를 읽기 전에 테스트를 먼저 읽는 것도 도움이 됩니다. 왜냐하면 변경 사항이 무엇을 해야 하는지에 대한 아이디어가 있기 때문입니다.

Speed of Code Reviews

• 왜 코드 리뷰가 빨라야 하는가?

  • 개인의 발전 속도도 중요하지만, 팀 전체의 발전 속도만큼 중요하지는 않다.
  • 검토자가 동일한 실질적인 변경사항(코드 상태를 개선하는 변경사항)을 요청할 때,
    • 개발자가 업데이트할 때마다 신속하게 대응하면 불만사항이 사라지는 경향이 있다.
  • 코드 검토 과정에 대한 대부분의 불만은 실제로 프로세스를 더 빠르게 함으로써 해결된다.

• 코드 리뷰가 얼마나 빨라야 하는가?

  • CL을 받으면 최대한 자기일 보다는, 리뷰를 먼저 해 주는 것이 좋습니다.
  • 일반적인 규칙은 (처음이든 update한 버전이든 상관 없이) CL을 받으면 24시간 내에는 조금이라도 리뷰를 보내주는 것입니다.
  • CL이 너무 크고 어렵다던지 해서, 24시간 내에 리뷰를 못 보내줄 것 같으면, 이메일이나 offline으로 사정을 미리 얘기하는 것이 좋습니다.
  • 반대로, CL 작성자가 리뷰를 받은 후에, 언제까지 수정 버전을 보내줄 것인지에 대한 규칙은 없습니다.

• Fast Responses

  • CL 이 전체 검토를 통과하고 제출되는데 걸리는 시간이 보다 더 중요한 것은, reviewer의 응답 시간이다.
  • 모든 것을 리뷰하지 못하고, 부분부분 나눠서 리뷰하더라도, 빠른 응답은 매우 중요하다.
  • 빠른 response의 최종 결과로, 충분한 코드 검토가 도출되야 한다.

• Large CLs

  • 만약 누군가가 너무 커서 언제 그것을 검토할 시간이 있을지 확신할 수 없는 코드 검토를 보낸다면,
    • 당신의 일반적인 반응은 개발자에게 한꺼번에 검토해야 하는 하나의 거대한 CL 대신,
    • 서로 기반을 둔 여러 개의 더 작은 CL로 CL을 분할하도록 요청하는 것이어야 한다.

How to Write Code Review Comments

• 요약
  • 친절하게
  • 이유를 명시하라.
  • 대안을 제시하는 것 vs 지적만 하는 것 에 대한 균형을 맞춰라.
  • 개발자가 복잡성을 git에 글로 설명하는 대신, 코드를 단순화하거나 코드 주석을 추가하도록 권장합니다.
• Giving Guidance
  • 대안 솔루션의 세부 설계를 수행하거나, 개발자를 위한 코드를 작성해서 보여줄 필요는 없다.
  • 일반적으로 문제를 지적하는 것과 직접적인 지침을 제공하는 것 사이에서 적절한 균형을 유지해야 합니다.
  • 그러나 때로는 직접적인 지시, 제안 또는 심지어 코드가 더 도움이 된다.
  • CL에서 마음에 드는 것을 보면 그것들도 댓글로 달아주세요!
• label
  • Nit
    • 사소하다. 기술적으로는 해야하지만, 큰 impact는 없다.
  • Optional(or Consider)
    • 좋은 아이디어를 제공할 것이지만, 엄격히 요구되는 것은 아니다.
  • FYI
    • 이 CL에서 하는 것을 기대하진 않지만, 미래에 도움이 될 만한 것들
• Accepting Explanations
  • 코드리뷰 툴에만 작성된 설명은, 향후 code를 다시 읽을 때 별 도움이 되지 않습니다.
  • 코드에 주석을 추가하거나, 코드를 읽기 쉽게 refactoring해서 CL에 반영하는 것이 낫습니다.
  • 최소 TODO로 코드에 적어놓아라.

Handling Pushback in Code Reviews(author이 코드 리뷰에 대한 대응을 미루는 경우 대응법)

• 현재 CL에 관련된 review만 해라.
• 커멘트를 나중에 다른 CL에서 처리하겠다는 것은 -> 코드 품질을 떨어뜨리는 지름길이다.
  • 최소 TODO로 코드에 적어놓아라.

코드 reviewer은 무엇을 보아야 하는가?

• 코드를 읽는 것이 너무 어려워서 검토 속도가 느려진다면, 개발자에게 이 사실을 알리고 검토하기 전에 개발자가 명확하게 설명할 때까지 기다려야 합니다.

  • 만약 당신이 코드를 이해할 수 없다면, 다른 개발자들도 이해하지 못할 가능성이 높다.

• (정말 중요) 코드리뷰는 훌륭한 것이나 좋은 관행에 대한 격려나 감사도 제공해야 한다.
  

  • Design

    • 코드 리뷰의 가장 중요한 부분: CL의 전체 design 보기
    • 코드가 잘 디자인 되었고, 우리 시스템에 부합하는가?
      • CL의 다양한 조각들이 상호작용 하는 방식
      • CL이 codebase에 속해야 할까, 아니면 library에 속해야 할까?
      • CL이 나머지 system에 잘 통합되는가?
      • 이 기능을 지금 넣기 좋은 시점인가?

  • Functionality

    • 코드가, 작성자가 의도한대로 작동하는가?
    • 코드가 사용자(서비스 user와 개발자 모두)들에게 좋은 방식으로 작동하는가?
    • author가 테스트 코드를 거쳤겠지만, reviewer들을 엣지 케이스나, concurrency 문제를 더 고려해봐야 한다.
    • 코드 검토 중에 기능성에 대해 생각하는 것이 특히 중요한 또 다른 시기는 이론적으로 교착 상태나 경쟁 조건을 야기할 수 있는 일종의 병렬 프로그래밍이 CL에서 진행되고 있는지 여부이다.

  • Complexity

    • 코드가 더 단순해질 순 없는가?
    • 복잡하다는 것은 코드를 읽을 때, 빨리 이해할 수 없다는 것을 의미한다.
    • 미래에 코드를 다시 보았을 때, 다른 개발자들이 쉽게 이해하고, 이 코드를 쓸 수 있는가?
    • 복잡성의 특정 유형
      • 코드를 필요 이상으로 일반화한 경우
      • 시스템에 현재 필요하지 않은 기능을 추가한 경우
    • 개발자가 미래에 해결해야 한다고 추측하는 문제가 아니라, 지금 해결해야 한다고 생각하는 문제를 개발자가 해결하도록 권장해야 합니다.

  • Tests

    • 해당 코드가 "정확하고 유용한 자동화 테스트"를 포함하는가?
    • 테스트 코드 역시 복잡하면 안된다.
    • 바쁜 긴급 상황이 아니면, test 코드를 추가해야 한다.
    • unit / integration / end-to-end test 를 모두 고려하자.

  • Naming

    • 변수, 클래스, method에 명확한 이름을 선택했는가?

  • Commnets

    • 커멘트가 명료하고 유용한가?
    • 중요 주석은 class, module, function의 문서와는 다른 개념이다. (후자는 code의 목적과 사용법과 어떤 행동을 하는지를 모두 포함한 문서다.)
    • 일반적으로 주석은, 코드가 존재하는 이유를 설명할 때 유용하며, 어떤 코드가 무엇을 하는지 설명해서는 안됩니다.(복잡한 알고리즘이 아니라면)
    • 주석은 코드가 포함할 수 없는 정보를 위한 것이다.
    • 어떤 코드가 스스로를 설명할 만큼 충분히 명확하지 않다면, 코드는 더 단순해져야 한다.

  • Style

    • 코드가 https://google.github.io/styleguide/ 를 따르고 있는가?
    • 스타일에 없는 review를 적용하려면 "Nit"를 붙여라.

  • Documentation

    • 개발자가 관련된 documentation을 업데이트 했는가?
    • generally in g3doc