컴퓨터공학 300 주제 시리즈의 190번째 글이다. 전체 지도는 여기.

한 줄 요약

코드 리뷰는 변경을 합치기 전에 작성자가 아닌 사람이 읽고 검토하는 과정이다. 목표는 버그 찾기만이 아니라 코드베이스 전체의 건강을 유지하고 지식을 나누는 것이며, 기준은 “완벽한가” 가 아니라 “합치면 지금보다 확실히 나아지는가” 다.

왜 필요한가

작성자는 자기 코드의 의도를 알기 때문에, 코드가 실제로 무엇을 하는지보다 무엇을 하려 했는지를 읽는다. 그래서 자기 버그를 잘 못 본다. 두 번째 눈이 필요하다.

리뷰가 주는 것은 결함 발견만이 아니다.

  • 지식 공유: 리뷰어는 코드베이스의 다른 부분을 배우고, 작성자는 더 나은 관용구를 배운다. 한 사람만 아는 코드가 줄어든다.
  • 일관성: 팀의 설계 원칙과 스타일이 코드 전체에 고르게 퍼진다.
  • 설명 책임: 누군가 읽는다는 사실 자체가 더 명확한 코드와 커밋 메시지를 쓰게 만든다.

핵심 개념

리뷰의 기준: “확실히 나아지는가”

Google 이 공개한 코드 리뷰 가이드는 리뷰의 기준을 이렇게 정리한다. 변경(CL)이 완벽하지 않더라도, 작업 중인 시스템의 전반적인 코드 건강을 확실히 개선하는 상태라면 승인하는 쪽을 택해야 한다(The Standard of Code Review).

완벽주의 리뷰는 두 가지 비용을 만든다. 변경이 오래 묶여 작성자의 맥락이 식고, 리뷰를 피하려고 변경을 크게 뭉쳐 올리게 된다. 반대로 “대충 승인” 은 코드 건강을 조금씩 갉아먹는다. 기준은 그 사이, 이전보다 나빠지게 하지는 않는다는 선이다.

같은 문서는 사소한 지적에 Nit: 접두어를 붙여 “고치면 좋지만 필수는 아님” 을 표시하라고 권한다. 지적의 무게를 명시하면 작성자가 무엇을 반드시 고쳐야 하는지 헷갈리지 않는다.

무엇을 보는가

Google 가이드의 리뷰어가 볼 것 항목을 우선순위대로 묶으면 대략 이렇다.

순서 항목 질문
1 설계 이 변경이 여기 있는 게 맞나? 시스템과 잘 맞물리나?
2 기능 작성자 의도대로 동작하나? 사용자에게 좋은가? 동시성 문제는?
3 복잡도 필요 이상으로 복잡하지 않나? 미래를 위한 과잉 일반화는?
4 테스트 적절한 테스트가 있고, 테스트가 실제로 실패할 수 있나?
5 이름·주석 이름이 의도를 드러내나? 주석이 “왜” 를 설명하나?
6 스타일 팀 스타일 가이드를 따르나? (가능하면 도구에 맡긴다)

설계에서 근본적인 문제가 보이면 세부 스타일 지적은 미룬다. 어차피 크게 바뀔 코드다.

작게, 빨리

리뷰 품질은 변경 크기에 크게 좌우된다. 수천 줄짜리 PR 은 아무도 꼼꼼히 읽지 않는다. Google 가이드는 작은 변경을 권하며, 작은 변경이 더 빨리, 더 철저히 리뷰되고, 버그가 덜 들어가며, 되돌리기도 쉽다고 설명한다.

속도도 중요하다. 같은 가이드는 리뷰 요청에 응답하는 데 걸리는 시간이 최대 1영업일이어야 한다고 말한다(Speed of Code Reviews). 응답이 빠르다는 것은 리뷰를 다 끝낸다는 뜻이 아니라, 최소한 첫 피드백을 준다는 뜻이다.

코멘트 쓰는 법

  • 사람이 아니라 코드에 대해 말한다. “왜 이렇게 짰어요?” 보다 “이 부분은 동시 호출 시 경쟁 조건이 생길 수 있을 것 같습니다” 가 낫다.
  • 이유를 붙인다. “이렇게 바꾸세요” 만 쓰면 배울 것이 없다.
  • 무게를 표시한다. 필수, 제안, Nit:, 질문을 구분한다.
  • 칭찬도 쓴다. 좋은 테스트, 깔끔한 추상화를 봤으면 말한다. 무엇을 계속해야 하는지도 피드백이다.

플랫폼의 장치

GitHub 의 PR 리뷰는 Comment(의견만), Approve(승인), Request changes(변경 요청) 세 가지 상태로 리뷰를 제출한다. CODEOWNERS 파일로 경로별 책임자를 지정하면 해당 경로가 바뀐 PR 에 리뷰어가 자동으로 요청된다. 보호 브랜치 규칙과 함께 쓰면 “코드 소유자 승인 없이는 병합 불가” 를 강제할 수 있다.

# .github/CODEOWNERS 예시
*.py            @backend-team
/deploy/        @platform-team
/docs/          @tech-writers

사람과 기계의 분담

기계에 맡길 것 사람이 볼 것
포매팅, 린트, 타입 검사 설계가 맞는가
단위·통합 테스트 통과 요구사항을 제대로 이해했는가
알려진 위험 패턴(비밀값, SQL 조립) 이름과 추상화가 적절한가
커버리지 변화 테스트가 의미 있는 것을 검증하는가

기계가 할 수 있는 지적을 사람이 하면 리뷰어의 주의력이 거기서 소모된다.

직접 해 보기

파이썬 difflib으로 변경 전후의 diff 를 만들고, 추가된 줄만 검사하는 작은 자동 리뷰어를 만든다. CI 의 정적 분석 단계가 하는 일을 축소한 것이다.

import difflib, re

before = '''def get_user(db, user_id):
    row = db.query("SELECT * FROM users WHERE id = ?", (user_id,))
    return row
'''.splitlines(keepends=True)

after = '''import requests

API_KEY = "example-not-a-real-key"

def get_user(db, user_id):
    print("debug", user_id)
    row = db.query(f"SELECT * FROM users WHERE id = {user_id}")
    # TODO: 캐시 붙이기
    requests.post("https://audit.example.com", json={"id": user_id})
    return row
'''.splitlines(keepends=True)

diff = list(difflib.unified_diff(before, after, "a/users.py", "b/users.py"))
print("".join(diff))

RULES = [
    (r"(api_key|secret|password|token)\s*=\s*[\"'][^\"']+[\"']", "BLOCK", "비밀값이 코드에 들어 있다"),
    (r"f[\"'].*(SELECT|INSERT|UPDATE|DELETE).*\{",            "BLOCK", "SQL 을 f-string 으로 조립한다(인젝션 위험)"),
    (r"^\s*print\(",                                           "NIT",   "디버그 print 가 남아 있다"),
    (r"TODO",                                                  "INFO",  "TODO 는 이슈 번호와 함께 남기자"),
    (r"requests\.(get|post)\((?!.*timeout)",                   "WARN",  "HTTP 호출에 timeout 이 없다"),
]

added = [(i, l[1:]) for i, l in enumerate(diff) if l.startswith("+") and not l.startswith("+++")]
print(f"추가된 줄 {len(added)}개, 삭제된 줄 "
      f"{sum(1 for l in diff if l.startswith('-') and not l.startswith('---'))}개\n")
for _, line in added:
    for pattern, level, msg in RULES:
        if re.search(pattern, line, re.IGNORECASE):
            print(f"[{level:5s}] {line.strip()[:50]:50s} -> {msg}")

실행 결과(정렬 공백은 터미널 글꼴에 따라 어긋나 보일 수 있다):

--- a/users.py
+++ b/users.py
@@ -1,3 +1,10 @@
+import requests
+
+API_KEY = "example-not-a-real-key"
+
 def get_user(db, user_id):
-    row = db.query("SELECT * FROM users WHERE id = ?", (user_id,))
+    print("debug", user_id)
+    row = db.query(f"SELECT * FROM users WHERE id = {user_id}")
+    # TODO: 캐시 붙이기
+    requests.post("https://audit.example.com", json={"id": user_id})
     return row

추가된 줄 8개, 삭제된 줄 1개

[BLOCK] API_KEY = "example-not-a-real-key"                 -> 비밀값이 코드에 들어 있다
[NIT  ] print("debug", user_id)                            -> 디버그 print 가 남아 있다
[BLOCK] row = db.query(f"SELECT * FROM users WHERE id = {u -> SQL 을 f-string 으로 조립한다(인젝션 위험)
[INFO ] # TODO: 캐시 붙이기                                     -> TODO 는 이슈 번호와 함께 남기자
[WARN ] requests.post("https://audit.example.com", json={" -> HTTP 호출에 timeout 이 없다

기계는 패턴을 잘 잡는다. 그러나 이 diff 의 가장 큰 문제는 기계가 못 잡았다. 사용자를 조회하는 함수가 외부 감사 서버로 요청을 보내는 부수 효과를 갖게 됐다. 조회 함수가 네트워크 호출을 하게 된 것이 설계상 맞는지, 실패하면 조회도 실패해야 하는지는 사람만 판단할 수 있다. 위 표의 “설계” 가 1순위인 이유다.

현업에서는

  • PR 설명이 리뷰의 절반이다. “무엇을, 왜 바꿨고, 어떻게 확인했는가” 를 PR 본문에 쓰면 리뷰어가 코드에서 의도를 역추적하지 않아도 된다. 화면 변경이면 전후 스크린숏을 붙인다.
  • 리뷰 대기 시간이 개발 속도를 정한다. 코드를 짜는 데 하루, 리뷰를 기다리는 데 사흘이면 팀 속도는 리뷰가 결정한다. 많은 팀이 “오전·오후 한 번씩 리뷰 큐 비우기” 같은 규칙을 둔다.
  • 인프라 변경도 리뷰한다. 홈랩 클러스터라도 매니페스트와 Helm 값 변경을 PR 로 올리고 diff 를 본 뒤 반영하면, “누가 언제 왜 이 리소스 제한을 바꿨나” 가 기록으로 남는다. GitOps 의 출발점이 여기다.
  • AI 리뷰 도구는 첫 번째 필터다. 자동 리뷰 도구가 흔한 실수를 먼저 걸러 주면 사람 리뷰어는 설계와 요구사항 이해에 집중할 수 있다. 다만 최종 승인 책임은 사람에게 남는다.

확인 문제

  1. Google 코드 리뷰 가이드가 제시하는 승인 기준을 한 문장으로 쓰라.
  2. 작은 PR 이 큰 PR 보다 나은 이유를 두 가지 들라.
  3. 리뷰 코멘트에 Nit: 을 붙이는 목적은?
  4. CODEOWNERS 파일은 무슨 일을 하는가?
  5. 위 자동 리뷰어가 잡지 못한 설계 문제는 무엇인가?

풀이

  1. 변경이 완벽하지 않더라도 시스템의 전반적인 코드 건강을 확실히 개선한다면 승인한다.
  2. 더 빠르고 철저하게 리뷰되고, 버그가 덜 들어가며, 문제가 생겼을 때 되돌리기 쉽다(이 중 둘).
  3. 사소한 개선 제안이라 반드시 고칠 필요는 없다는 무게를 명시해, 필수 지적과 구분하기 위해서다.
  4. 경로별 코드 소유자를 지정해, 그 경로를 바꾸는 PR 에 소유자를 리뷰어로 자동 요청한다. 보호 브랜치 규칙과 함께 소유자 승인을 병합 조건으로 만들 수 있다.
  5. 조회 함수가 외부 서버로 요청을 보내는 부수 효과를 새로 갖게 된 점. 책임 분리와 실패 처리 방식은 사람이 판단해야 한다.

더 읽을거리 (References)