삼 년 전, 나는 단 Weekend 동안 우리 회사에 $47,000의 수익 손실을 초래한 pull request를 승인했다. 코드 자체는 괜찮아 보였다. 테스트는 통과했다. 논리도 괜찮았다. 하지만 우리는 구독 청구 시스템의 시간대 변환 처리를 하는 방식에서 미묘한 점을 놓쳤고, 특정 지역의 고객이 두 번 청구당하는 문제가 생겼다.
💡 주요 요점
- 첫 30초: PR 설명이 알려주는 내용
- 사이즈가 중요하다: 400라인 규칙
- 비즈니스 로직 계층: 대부분의 버그가 존재하는 곳
- 에러 처리: 좋은 코드와 훌륭한 코드의 차이
그 사건은 내가 코드를 검토하는 방식을 영원히 바꿨다. 나는 사라 첸입니다, 그리고 지난 10년 동안 세 개의 다른 SaaS 회사에서 수석 엔지니어링 매니저로 일하며 주당 평균 15-20개의 pull request를 검토해왔습니다. 제 경력 동안 대략 8,000개의 PR을 다뤘습니다. 나는 버그를 포함한 뛰어난 코드와 생산에서 수년간 완벽하게 실행된 지저분한 코드를 보았습니다. 코드 리뷰는 문법 오류를 찾는 것이 아니라—당신의 린터가 이 일을 처리합니다—경험만이 밝힐 수 있는 보이지 않는 문제를 잡는 것이라고 배웠습니다.
이 글은 내가 시작했을 때 가졌으면 좋았던 체크리스트입니다. 포괄적이지는 않지만—어떤 체크리스트도 포괄적일 수는 없지만—내가 알아차릴 수 있는 패턴, 내가 스스로에게 물어보도록 훈련한 질문들, 그리고 내 팀을 수많은 생산 사고에서 구해준 정신 모델을 반영합니다.
첫 30초: PR 설명이 알려주는 내용
나는 코드의 한 줄도 보기 전에 PR 설명을 30초 동안 읽습니다. 이게 뒤바뀐 것처럼 보일 수 있지만, 문제가 있는 PR의 60%가 나쁜 소통을 통해 즉시 드러난다는 것을 알게 되었습니다. 좋은 PR 설명은 세 가지 질문에 대답해야 합니다: 무엇이 변경되었는지, 왜 변경되었는지, 그리고 무엇이 잘못될 수 있는지.
"버그 수정" 또는 "구성 요소 업데이트"라는 설명을 보면, 나는 즉시 이 리뷰가 오래 걸릴 것이라는 것을 압니다. 작성자가 자신의 변경사항의 의미를 잘 생각하지 않았다는 것이고, 이는 내가 그 일을 대신해야 한다는 것을 의미합니다. 반대로, "사용자 인증을 세션 쿠키 대신 JWT 토큰을 사용하도록 리팩토링했습니다. 이것은 피크 시간 동안 데이터베이스 부하를 40% 줄이지만, 클라이언트가 토큰 갱신을 처리해야 합니다. 위험: 기존 모바일 앱 버전(< 2.3)은 재인증을 받아야 합니다."라는 설명을 보면, 나는 작성자가 숙제를 했음을 압니다.
나는 설명에서 특정 지표를 찾습니다. 모호한 개선이 아니라 실제 숫자가 필요합니다. "성능 개선"은 아무 의미가 없습니다. "1000개의 동시 요청에서 사용자 프로필 엔드포인트의 API 응답 시간을 340ms에서 180ms로 줄였습니다."는 내가 작성자가 자신의 작업을 측정하고 그 영향을 이해했음을 알게 합니다. 내 경험상, PR 설명에 지표를 포함한 개발자는 전반적으로 더 깊이 있는 코드를 작성합니다.
설명은 또한 관련된 맥락에 연결되어야 합니다—티켓, 디자인 문서, 접근 방식이 논의된 Slack 스레드. 내가 단독으로 PR을 검토하고 더 넓은 맥락을 이해하지 못하면 많은 것들을 놓칠 것입니다. 한 번은 "간단한 리팩토링"을 승인한 적이 있는데, 문서화되지 않은 종속성 때문에 중요한 통합을 고의로 깨뜨렸던 적이 있습니다. 이 종속성은 세 달 된 이메일 서신에서만 언급되었습니다.
PR 설명의 적신호는: 전혀 설명이 없는 것, 코드 변경보다 긴 설명(보통 오버 엔지니어링을 나타냄), "WIP" 또는 "초안"이라는 설명이 있지만 PR은 검토 준비가 완료로 표시되는 경우, "이게 맞는 접근인지 확신이 없다"라는 문구가 포함된 설명(왜 나에게 검토를 요청하는 건가요?)입니다.
사이즈가 중요하다: 400라인 규칙
나는 엄격한 규칙이 있습니다: 실제 코드 변경이 400라인을 넘는 PR은 철저히 검토하지 않습니다(생성된 코드, package-lock 파일 및 테스트 픽스는 제외). 이는 임의의 것이 아닙니다. Cisco와 SmartBear의 연구에 따르면, 400라인 이후에 코드 리뷰의 효과가 급속도로 떨어지며, 내 경험으로도 이를 확인할 수 있습니다. 약 200라인의 코드를 검토한 뒤부터, 내 두뇌는 세부사항을 지나치게 어지럽니다. 400라인에 도달하면 나는 기본적으로 대충 훑어보는 수준입니다.
"코드 리뷰는 문법 오류를 찾는 것이 아니다—당신의 린터가 그 일을 한다. 경험만이 밝혀낼 수 있는 보이지 않는 문제를 잡는 것이다."
누군가 1,200라인의 PR을 제출하면, 나는 그들을 나누라고 요청합니다. "모두 관련이 있다"고 해도 상관하지 않습니다. 큰 PR은 버그가 숨어 있는 곳입니다. 리뷰어들이 지치고 600라인 주변에서 주의를 기울이지 않아 중요한 보안 취약점이 대규모 리팩토링 PR에서 놓치는 것을 보았습니다. 취약점은 847라인에 있었습니다.
물론 예외가 있습니다. 때때로 파일을 옮기거나 생성된 코드를 업데이트하거나 새로운 린팅 표준을 충족하기 위해 대규모 변경을 수행해야 할 때가 있습니다. 그런 경우, 나는 작성자에게 기계적 변경과 논리적 변경을 분리하도록 요청합니다. 파일 이동을 하나의 PR로 제출하고, 실제 논리 변경을 다른 PR로 제출하세요. 이렇게 하면 두 PR 모두 검토하기가 더 쉽고 문제가 발생했을 때도 되돌리기 쉬워집니다.
또한 추가된 코드와 삭제된 코드의 비율에 주의합니다. 내 경험에 비추어 볼 때, 가장 좋은 PR은 음의 라인 수를 가지고 있습니다—코드베이스를 작게 만들면서 무언가를 이룹니다. 500라인을 추가하고 50라인을 삭제하는 PR을 보면 의심이 갑니다. 복잡성을 추가하고 있나요? 기존 기능을 중복하고 있나요? 작성자는 코드베이스에 무엇이 있는지 알고 있나요?
사이즈 문제는 개별 파일에도 적용됩니다. 만약 PR이 30개의 다른 파일을 건드린다면, 총 라인 수가 합리적이라 하더라도 이는 적신호입니다. 이는 변경이 잘 조정되지 않았거나 코드베이스가 결합 문제를 가지고 있음을 시사합니다. 어느 쪽이든, 이는 추가적인 검토가 필요합니다.
비즈니스 로직 계층: 대부분의 버그가 존재하는 곳
10년 동안의 코드 리뷰 후, 버그가 어디에 숨는지 알 수 있습니다: 비즈니스 로직 계층, 특히 엣지 케이스와 상태 전환에서. UI 구성 요소에는 없습니다. 데이터베이스 쿼리에도 없습니다. 실제 비즈니스 규칙을 구현하는 코드에 있습니다.
| PR 설명 품질 | 내용 | 검토 시간 | 위험 수준 |
|---|---|---|---|
| 불량 | "버그 수정" 또는 "구성 요소 업데이트" | 2~3배 더 오래 | 높음 |
| 적당함 | 기본적인 무엇과 이유, 위험 분석 없음 | 정상 | 중간 |
| 우수함 | 명확한 무엇, 이유 및 잠재적 위험 파악됨 | 효율적 | 낮음 |
| 탁월함 | 포괄적인 문맥, 논의된 트레이드오프, 엣지 케이스 주목됨 | 빠름 | 매우 낮음 |
비즈니스 로직을 검토할 때, 나는 가정을 찾고 있습니다. 모든 가정은 잠재적인 버그입니다. 나는 한 번 할인 계산 시스템의 코드를 검토한 적이 있는데, 모든 할인은 비율로 가정하고 있었습니다. 코드는 마케팅이 "$10 할인" 프로모션을 제공하고 싶어할 때까지 완벽하게 작동했습니다. 누군가가 할인 금액을 나누는 바람에 시스템이 다운되었습니다. 0과 1 사이의 숫자를 예상했을 때 10으로 나누면 매우 잘못된 결과가 발생합니다.
나는 스스로에게 묻습니다: 경계에서 무슨 일이 발생하나요? 입력이 0이면 어떻게 되나요? 음수이면? null이면? 빈 문자열과 null 문자열의 차이는? 배열이 비어있으면? 사용자가 권한이 없으면? 모든 권한이 있으면? 데이터베이스가 결과를 반환하지 않으면? 백만 개의 결과를 반환하면?
나는 비즈니스 로직에서 마법 숫자를 찾습니다. if (user.loginAttempts > 5)를 보면, 나는 묻습니다: 왜 5인가요? 문서화되었나요? 구성할 수 있나요? 특정 사용자 유형에 대해 3으로 변경하고 싶으면 어떻게 됩니까? 비즈니스 로직의 마법 숫자는 기술 부채가 될 준비가 되어 있습니다.
상태 기계는 또 다른 일반적인 버그 원인입니다. 상태 전환(주문 상태, 사용자 생애주기, 워크플로 단계)을 관리하는 코드를 보면, 나는 내 머리 속에 상태 다이어그램을 그립니다. 모든 전환이 고려되어 있나요? 유효하지 않은 상태로 들어갈 수 있나요? 나는 한 번 사용자가 "활성"과 "정지" 상태를 동시에 가질 수 있는 버그를 잡은 적이 있습니다. 이유는 한 상태를 설정하는 코드가 다른 상태를 확인하지 않았기 때문이었습니다.
나는 또한 여러 계층에 걸쳐 퍼져 있는 비즈니스 로직에도 주의를 기울입니다. UI에 검증이 있고, API에 더 많은 검증이 있으며, 데이터베이스 계층에서도 더 많은 검증이 있다면, 우리는 불일치를 겪게 될 것입니다. 비즈니스 규칙은 한 곳에 존재해야 하며, 그곳은 분명해야 합니다.