Skip to content

Latest commit

 

History

History
172 lines (169 loc) · 9.72 KB

code-review2.md

File metadata and controls

172 lines (169 loc) · 9.72 KB
Raw

(해당 글은 백명석님의 '코드 리뷰는 왜 해야하나' 피피티를 보며 손으로 직접 타이핑 하여 옮기면서 공부하였습니다.)

  • code review 2번째 글로 첫번째 글을 미리 보고 보는게 좋습니다.
  • link) code review-1

코드 리뷰 기법들

  • 지루한 작업은 컴퓨터로 처리
    • 코드를 읽는 것은 인지적으로 부담되는 고수준의 집중이 요구되는 작업이다.
    • 컴퓨터가 할 수 있는 일에 이런 노력을 낭비하지 말라.
    • Formatting Tool
      • 공백, 들여쓰기 오류 등..
      • 별도의 커밋/PR로 분리. 리뷰 불필요를 기술해서 리뷰를 생략할 수 있도록
  • 리뷰는 즉시 시작
    • 코드 리뷰를 높은 우선순위로 다뤄라.
    • 저자는 리뷰 종료될 때까지 대기 (Blocked) 함
    • 리뷰를 바로 시작하면 선순환 된다.
    • PR의 Size 와 Complexity가 중요
    • 작고 범위가 좋은 PR
    • 쉽고 상쾌하기 리뷰하기 좋음
    • 빠르게 리뷰 수행.
    • 리뷰어를 생각하여 PR을 보내야 한다. 잘 쪼개진 PR은 리뷰어의 부담을 덜어준다.
    • 커밋에 목적은 작고 모듈러 할 수록 이해하기 좋다.
    • 리뷰 라운드의 최대 시간은 하루
    • 우선순위 높은 업무로 1일 내 불가하면 다른 리뷰어 지정
    • 월 1회 이상 재지정을 해야한다면 속도를 줄여서 건강한 개발 practices를 유지할 수 있어야함.
  • 고수준으로 시작, 저수준으로 내려가라
    • 리뷰 라운드에서 더 많은 의견을 남길 수록, 저자가 당황할 위험 커짐.
    • 하나의 라운드에 20-50개 정도의 의견은 위험의 시작
    • 초기 라운드에서는 고수준 피드백으로 제한
    • 인터페이스 클래스의 재설계, 복잡한 함수의 분리 등
    • 고수준의 피드백이 처리된 후에 저수준 이슈를 처리
    • 변수명 변경, 주석을 명확하게 하는 것 등
    • 예제 코드 제공에 관대하라.
  • 저자를 기분 좋게 하기 위한 방법
    • 리뷰 중에 선물 주기 (코드 예제)
    • “List comprehension으로 간단히 할 수 있지 않을까요?”
    • List comprehension을 모른다면 20분을 헤매야 한다..
    • 너무 긴 예제 는 관대한 것이 아니라 억압적으로 보임.
    • 라운드당 2-3개의 코드 예제로 제한
    • 모든 PR(Pull Request)에 예제를 제공하면 저자가 코드를 작성할 수 없다고 생각한다는 신호.---
  • 절대 “너”라고 하지마라.
    • 저자의 방어 유발을 최소화하는 방법으로 피드백
    • 비판의 대상은 코드. 저자가 아님
    • “너” : 저자의 주의를 코드에서 자신으로 바꿈
    • “너”만 빼라(저자에 대한 판단 -> 단순한 정정)
    • “You misspelled ’successfully” ->
    • “Successfully -> successfully”
    • 주어만 빼라.
    • 하는 것을 제안합니다. -> 하는게 어떨까요?
    • 피드백은 명령이 아니라 요청으로 표현해라.
    • 일상에서 동료에게 명령하지 않음.
    • 하지만 리뷰에서는 강압적인 명령을 종종 발견됨
    • ex. 이 클래스를 별도의 파일로 분리하라 (X)
    • ex. 이 클래스를 별도의 파일로 분리할 수 있을까요?
    • 의견이 아니라 원칙에 기반하여 피드백하라.
  • 저자에게 의견을 줄 때는
    • “제안하는 변경”과 “변경의 이유”를 모두 설명하라.
    • Ex) 이 클래스를 2개로 분리해야 해요. -> 지금 이 클래스는 파일 다운로드와 파싱의 2가지 책임을 가지고 있어요. 다운로더와 파서 2개의 클래스를 분리하여 SRP를 준수하면 어떨까요?
    • SW는 과학인 동시에 예술???
      • 항상 원칙에 기반하여 정확히 뭐가 잘 못 되었는지 언급할 수 있는 것은 아니다.
      • 단지 그냥 보기 싫거나 직관적이지 않을 수 있다.
      • 무엇을 할 수 있을지 객관적으로 설명하라.
      • Ex. This is confusing -> I found this hard to understand
  • 한 두 등급만 코드 레벨을 올리는 것을 목표
    • D 등급의 PR을 받으면 저자가 C나 B 등급을 받도록 도와라.
    • Letter Grade
      • Make it work, make it right, make It fast
    • 완전하지는 않아도 충분히 좋은 코드가 되도록
    • F
      • 기능적으로 틀렸거나
      • 너무 복잡해서 정합성에 확신이 없는 상태
      • 승인을 보류하는 유일한 이유
      • 수 차례의 리뷰 라운드 후에도 코드가 F 상태인 경우
  • 반복적인 패턴에 대해서 피드백 제한하라
    • 저자의 실수가 동일한 패턴임을 식별 했다면 모든 경우를 언급하지 말라.
    • 동일 패턴에 대해서 2~3개 정도의 예를 언급하라.
    • 그 이상의 저자에게 개별 사례가 아니라 패턴에 대해서 수정을 요구하라
  • 리뷰의 범위를 존중하라
    • 자주 보이는 Anti-Pattern
    • PR 근처의 코드를 보고 저자에게 수정을 요청
    • Rule of thumb
      • PR에 포함되지 않은 라인은 리뷰 범위가 아님.
      • 예외 : PR이 둘러싼 코드에 영향을 미칠 때.
  • 큰 리뷰를 잘게 나눌 기회를 찾아라
    • 400 LOC 이상의 리뷰는 자겍 분리하도록 제안
    • 저자가 PR을 분리하는 번거로운 일에 불평할 수 있음
    • 분리를 위한 논리적 경계를 식별해서 짐을 덜어줘라
    • 예
      • 여러 파일에 걸쳐 변경했다면 파일별로 리뷰를 나눠라
      • 추상화 수준이 낮은 함수/클래스를 찾아 별도 리뷰로 분리 제안
      • 코드 품질이 낮을 때 리뷰 분리를 강조
      • 품질이 낮은 코드의 리뷰는 LOC가 늘면 기하급수적으로 어려워짐.
  • 진정한 칭찬을 해라
    • 대부분의 리뷰어가 코드의 잘못된 부분에만 집중
    • 하지만 리뷰는 긍정적 행위 강화 를 위한 값진 기회이기도 함
    • PR에서 좋은 변경이 있을 때마다
    • 오 이런 API가 있나요. 정말 유용해요
    • 정말 좋은 해결책이네요. 생각도 못했네요.
    • 함수를 분리한 것은 좋은 생각이에요. 훨씬 단순해졌어요.
    • 저자가 주니어 혹은 신규 입사자라면 리뷰에 매우 민감하고 __방어적__일 수 있음
    • 진심어린 칭찬은 리뷰어가 잔인한 감시자가 아니라 도와주려는 팀 동료 라는 것을 보여서 이런 긴장감을 낮춤.
  • 사소한 이슈만 남았다면 승인해라.
    • 모든 커맨트가 수정되는 것을 확인하고 나서야 승인하려는 오해 존재
    • 불필요한 코드 리뷰 라운드를 추가. 저자/리뷰어 모두 시간 낭비
    • 아래 조건 중 하나라도 만족되면 승인
      • 더 이상 코멘트가 없을 때
      • 남겨진 코멘트가 사소한 이슈
      • 남겨진 코멘트가 선택적 제안
  • 교착 상태를 적극적으로 처리해라
    • 코드 리뷰의 최악의 결과는 교착상태 (Stalemate)
    • 코맨트를 반영하지 않으니 승인 거부
    • 저자는 코멘트 반영을 거부
    • 만나서 얘기하라
    • 화상 혹은 만나서 논의
    • 텍스트 기반 의사소통은 상대가 인간이라는 것을 잊게 함
    • 인정하거나 Escalate하라
    • 교착상태가 길어지면 관계가 나빠짐
    • 그냥 승인하는 비용 - Agree to disagree
    • 저수준 코드를 무심코 승인하면 품질 좋은 SW를 만들 수 없음
    • 또한 동료와 너무 다퉈서 함께 일하지 않는다면 고수준의 품질을 얻을 수 없음
    • 인정이 불가한 경우
    • 저자에게 논의를 팀장이나 테크 리더에게 Escalation 권유
    • 다른 리뷰어에게 할당을 권유
  • 교착 상태로 부터 회복
    • 상황을 관리자와 논의하라
    • 휴식을 가져라.
    • 갈등 해결책을 학습하라.
  • 최악의 코드리뷰 사례
    • A가 B에게 PR을 보냄.
    • B가 보기에 의무감을 가지고 모든 이슈를 기록 (60군데) 많은 실수를 찾아낸 B는 훌륭한 리뷰어(?)
    • 몇 일 후 A는 Updated PR을 보냄
    • 코멘트에 대한 응답. 간단한 수정(변수명 변경, 오타 수정 등) 포함
    • 하지만 고수준 문제에 대한 언급 거부 (정의되지 않은 행위, 잘못된 형식을 갖는 입력, 6단계 까지 중첩되는 복잡한 제어문 등)
    • 화가 나서 새로운 코멘트를 보냄(전문가 다운 어투로)
    • 1주가 지난 지금도 동일 리뷰에 대해서 설왕설래 중
    • B가 A와 같은 사무실에 있기도 싫어함.
    • 3주가 지났는데 코드가 변하지 않았다.
  • 중재
    • 최고 시니어 개발자가 2개의 작은 라이브러리를 분리하는 것 (30-50LOC)에 대한 PR 작성을 A에게 요구하면 리뷰를 시작
    • A가 해당 업무를 수행하면 BoB는 즉각 승인
    • 그런 다음 Bob는 메인 PR(200 LOC를 정리하는)에 도달
    • 몇개의 작은 제안을 하고
    • PR을 승인
    • BOB의 전체 리뷰는 이틀만에 완료
    • 잘못한 점
      • A의 첫번째 리뷰
      • 평가 받는다고 느끼거나 방어적일 수 있다
      • 고수준 코멘트만으로 시작했으면 좋았다.
      • 많은 수의 코멘트로 공격 당하는 느낌을 받지 않도록
      • 리뷰어의 역할은 방해가 아니라 개선을 돕는 것임을 보여야 한다.
      • 코드 예제 제시, 변경 분에 대한 긍정적 언급 등
      • 교착상태가 길어지도록 방치
      • 만나서 깊은 갈등에 대해서 언급 or 관리자에게 요청 했어야
    • Bob이 잘한점
      • 처음에 리뷰를 분리한 것. 2개의 코드 블럭을 머지하면서 관계가 좋아짐.
      • 여전히 문제가 있었지만 문제는 작아졌고, PR을 관리하기 쉬워졌음
      • 리뷰를 완전하도록 억압하지 않앗다.