15. Inspections / Code Reviews

Learning Goals

• 다양한 Formality level을 가진 Peer review의 형태 이해
• Commit review system을 사용한 건설적인 Modern code review 참여
• Code review에서 좋은 Checklists의 이점과 속성 설명
• Code review의 사회적, 문화적 문제 완화
• 현대 기술 기업의 Commit review 동기와 이점 대조

Intuition

"Many eyes make all bugs shallow"
(많은 눈이 있으면 모든 버그는 얕아진다)
Linus' law

"Have peers, rather than customers, find defects"
(고객이 아닌 동료가 결함을 찾게 하라)
Karl Wiegers

Isn’t Testing Sufficient?

• 오류가 다른 오류를 은폐할 수 있음
• 완성된 구현만 테스트 가능 (특히 Scalability, Performance)
• Design documents는 테스트 불가
• Tests는 Code quality를 확인하지 않음
• 많은 품질 속성(예: Security, Compliance, Scalability)은 테스트하기 어려움

A Second Pair of Eyes

• 다른 배경과 경험 보유
• 정답에 대한 선입견 없음(No preconceived idea)
• "의도했던 것"에 편향되지 않음

Software Code Reviews

Code Reviews in GitHub

• Pull requests를 통해 [Git] repository에 Push한 변경 사항 공유
• Pull request가 열리면 Collaborators와 잠재적 변경 사항을 논의하고 검토하며 Repository에 Merge 되기 전 후속 Commits 추가 가능
• 다른 기여자들은 제안된 변경 사항 검토, Review comments 추가, PR 토론 참여, PR에 Commits 추가 가능

Code Reviews in VS Team

• Code check-in 전 Visual Studio를 사용해 팀원에게 리뷰 요청 가능
• 요청은 Team Explorer의 "My Work" 페이지에 표시됨

Google's Code Review Flow

1. 사용자가 변경 사항 작성 후 Snapshot(Patch 및 설명)을 Code review tool에 업로드
2. Author는 초기 Patch를 사용해 Automated review comments 적용 또는 Self-review 수행
3. Author가 Diff에 만족하면 하나 이상의 Reviewers에게 메일 발송 (알림 및 코멘트 요청)
4. Reviewers는 Tool에서 변경 사항을 열고 Diff에 Comments 게시 (명시적 해결 요청 또는 단순 정보성)
5. Author는 피드백 기반으로 수정 및 새 Snapshots 업로드 후 답변 (4, 5단계 반복 가능)
6. Reviewers가 최신 상태에 만족하면 "LGTM"(Looks Good To Me)으로 수락 (보통 1개의 LGTM 필요하나 관례상 전원 동의 요구 가능)
7. LGTM 후 Author는 모든 Comments를 해결하고 승인받은 경우 Codebase에 변경 사항 Commit

Code Review Goals

• Finding defects
  • Low-level 및 High-level issue 모두 포함 (Requirements/Design/Code)
• Code improvement
  • Readability, Formatting, Commenting, Consistency, Dead code 제거, Naming, Coding standards
• 대안 솔루션 식별
• Knowledge transfer
  • API 사용법, 라이브러리, Best practices, Team conventions, System design, "Tricks", 특히 Junior developers 교육
• Team awareness 및 Transparency
  • 다른 사람이 변경 사항 "Double check", 특정 개발자 또는 전체 팀에 공지("FYI")
• Shared code ownership
  • 비판과 변경에 대한 개방성, 개발자가 자신의 코드에 대해 덜 방어적이게 됨

Developers’ Expectations

Actual Outcomes

• 가장 빈번함: Code improvements (29%)
  • 58 Better coding practices
  • 55 Removing unused/dead code
  • 52 Improving readability
• 보통: Defect finding (14%)
  • 65 Logical issues (복잡하지 않은 논리 오류, 예: Corner cases, 설정 값, 연산자 우선순위)
  • 6 High-level issues
  • 5 Security issues
  • 3 Wrong exception handling
• 드묾: Knowledge transfer
  • 12 내부/외부 문서 포인터 등

Expectation vs Outcomes

• 기대와 결과 사이에 불일치 존재

Expectation/Outcome Mismatch

• Code reviews의 낮은 품질
  • Reviewers가 쉬운 오류(Formatting 등)만 찾음
  • 심각한 오류 놓침
• Understanding(이해)이 주된 과제
  • 변경 이유 이해
  • Code 및 Context 이해
  • 질문을 위한 Feedback channels 필요
• 결과물에 대한 Quality assurance 부재

Formal Inspection

• 70년대 IBM에서 아이디어 대중화
• 80년대 널리 채택, 많은 연구 진행 (때로는 Component testing 대체)
• 개발자 그룹이 Code나 다른 Artifacts를 공식적으로 검토하기 위해 만남
• 버그를 찾는 가장 효과적인 접근법
  • 일반적으로 Inspections로 60-90%의 버그 발견
• 비용이 많이 들고 노동 집약적

Inspection Team and Roles

• 보통 4-5명 (최소 3명)
• Author: 작성자
• Inspector(s): 결함(Faults) 및 광범위한 issue 발견
• Reader: 회의에서 Code나 Document 제시
• Scribe: 결과 기록
• Moderator: 프로세스 관리, 진행(Facilitates), 보고

Inspection Process

1. Planning
2. Overview
3. Preparation
4. Meeting
5. Rework
6. Follow-up

• 참여자
  • Author
  • Moderator
  • Inspectors (Scribe, Reader, Verifier 포함)

Inspection Steps

• Planning (Moderator 선정)
• Overview (간략): Author가 회의에서 Context 제시
• Preparation (1-2시간): 모든 Reviewer가 개별적으로 Code 검사
• Meeting (1시간)
  • Reader가 Code 제시
  • 모든 Reviewers가 issue 식별
  • 회의에서는 issue 발견만 하고, 솔루션 토의나 실제 issue 여부는 논쟁하지 않음
• Rework
• Follow-up (Verifier가 변경 사항 확인)

Checklists

• 무엇을 찾아야 할지 상기시킴
• 과거에 발견된 issue 포함
• 소수의 중요한 항목에 집중하는 것이 좋음
• 예시:
  • 사용 전 모든 변수 초기화 여부
  • 모든 변수 사용 여부
  • If/While 문의 조건 정확성
  • 각 Loop의 종료 여부
  • Function parameters의 올바른 타입 및 순서
  • Linked lists의 효율적 순회
  • 동적 할당된 메모리 해제
  • 예기치 않은 입력으로 인한 Corruption 가능성
  • 모든 가능한 Error conditions 처리 여부
  • Strings의 올바른 Sanitization 여부

Perspective-based Inspections

• 서로 다른 전문 분야나 Focus/Checklists를 가진 Inspectors 배치
  • 대안적 사고 패턴 장려
• Reviewers가 문서의 다른 위치에서 시작하도록 함
  • 같은 위치에서 집중력을 잃는 것 방지
• 특히 Preparation 단계에서 유효
• 출판된 데이터는 적지만 효과적인 관행으로 간주됨

Process Details

• Authors는 Code를 설명하거나 방어하지 않음 (객관적이지 않음)
  • Author $\neq$ Moderator $\neq$ Scribe, $\neq$ Reader
  • Author는 질문과 오해를 관찰하고 필요시 issue를 명확히 하기 위해 회의 참석
• Reader(optional)는 Code를 한 줄씩 읽으며 설명
  • Code를 소리 내어 읽으면 더 깊은 이해 필요
  • 해석을 언어화(Verbalizes)하여 해석의 차이 관찰 가능

Social Issues: Egos in Inspections

• Artifacts에 대한 Author의 자존감(Self-worth)
• 결함을 식별하되 대안 제시는 지양, Authors를 비판하지 않음
  • "너는 변수 a를 초기화하지 않았어" $\rightarrow$ "나는 변수 a가 어디서 초기화되는지 못 찾겠어"
• Code 방어 회피, 솔루션/대안 토론 회피
• Reviewers는 자신이 더 낫거나 똑똑함을 과시하지 말 것
• Guidelines가 없다면 Style 토론 회피
• 결함 해결 방식은 Author가 결정

Social Issues: Inspection Incentive

• Moderator는 토론을 진행하고 충돌 해결해야 함
• 회의에 경영진(Management) 포함 금지
• HR 평가에 사용 금지
  • "Inspection 중 5개 이상 버그 발견 시 Author에게 불이익" 같은 경우
  • 회피, 분할 제출, 결함 지적 회피, Pre-reviews 개최 등으로 이어짐
• Quality에 대한 책임은 Reviewers가 아닌 Authors에게 있음
  • "왜 고쳐, Reviewers가 찾을 텐데"라는 태도 방지

Root Cause Analysis

• 당면한 퍼즐 너머를 봄
• 이 문제를 피하기 위해 개발 프로세스를 개선하는 방법
  • 개발 프로세스 재구조화
  • 새로운 Policies
  • 새로운 개발 도구
  • 새로운 언어
  • 새로운 분석 도구

When to Inspect

• Milestones 이전
• 개발 중 점진적(Incremental) Inspections
  • 나중보다 초기가 좋음
    • 더 작은 조각, 향후 개발에 영향 줄 기회
  • 대규모 Code bases는 리뷰 비용이 높고 좌절감 유발 가능
• 분할 정복(Break down, divide and conquer)
• 중요 Components 집중
• 첫 세션의 Defect density를 식별하여 추가 Inspections 필요성 가이드

Reviews as Part of a Milestone

• Task X, Task Y 수행 후 Review 진행
• Milestone 도달 전 적절한 시점인지 확인
• Rework 과정을 거쳐 Milestone 달성

Guidelines for Inspections

• 다수 기업의 프로젝트 및 실험에서 수집됨
• 쉽게 측정 가능한 몇 가지 Metrics
  • Effort, Issues found, Lines of code inspected 등

Focus Fatigue

• 권장 사항: 세션당 60분 초과 금지

Inspection Speed

• 400 LOC/h 이상 시 리뷰가 얕아짐(Shallow)
• 권장 사항: 1시간 리뷰 세션에 400 LOC 미만 일정 잡기

Importance of Context

• Context dependencies가 적은 Code가 리뷰하기 쉬움
• Reviewers는 관련 파일 확인 필요
• Modularity (Small interfaces, High cohesion, Low coupling 등) 필요

Are Meetings Required

• 대부분의 issue는 Meeting이 아닌 Preparation 단계에서 발견됨
• 제안된 시너지 효과는 낮은 영향만 미침
• 주장: Meetings에서 발견되는 결함은 종종 더 미묘함(Subtle)

False Positives

• 발견된 issue의 약 25%는 False positives
• Meeting 중 토론 회피
• Meeting 중 혼란은 문서가 더 명확해야 한다는 지표(Indicator)

Self-checks Can Find Half the Issues

• Authors가 Inspection 전 문서를 Self-check함

The Goal Is Not To Be “Right” (it’s to save $)

"피로스의 승리(Pyrrhic victory)는 승리자에게 막대한 대가를 치르게 하여 패배와 다름없는 승리이다.
장기적 발전을 저해하거나 진정한 성취감을 무효화한다."

• 옳다는 것(Being right)이 항상 중요한 것은 아님
• "내 코드가 작동하지 않는다"는 말을 들었을 때 고려할 사고방식
  • "이 사람에게 왜 틀렸는지 말해줘야지" (X)
  • "내 코드가 왜 작동하는지 명확히 해야겠다" (O)

Inspections vs Reviews: Costs

• Formal inspections 및 Modern code reviews
  • Formal inspections는 매우 비쌈 (세션당 약 1 Developer-day)
  • Passaround review는 분산형, 비동기적(Asynchronous)
• Code reviews 대 Testing
  • Code reviews가 비용 효율적이라 주장됨
• Code reviews 대 버그를 찾지 못하는 비용 비교

Code Review by Formality

1. Inspection (가장 formal함)
2. Walkthrough
3. Pair Programming
4. Passaround ("Modern code reviews")
5. Ad hoc review

Differences Among Peer Review Types

Experience (Studies/Claims)

• Raytheon
  • "Rework" 비용 41%에서 20%로 감소
  • Integration effort 80% 감소
• Paulk et al. : Space shuttle software 수정 비용
  • Inspection 중 발견 시 $1
  • System test 중 발견 시 $13
  • Delivery 후 발견 시 $92
• IBM
  • Inspection 1시간이 Testing 20시간 절약
• R. Grady, HP의 효율성 데이터 (Defects/h)
  • System use: 0.21
  • Black box testing: 0.28
  • White box testing: 0.32
  • Reading/inspection: 1.06