코드리뷰 안티패턴들

• 코드 리뷰는 좋은 아이디어 같죠?
  • 코드 리뷰로 두 명의 개발자가 코드를 보면서 문제를 발견하고, 프로젝트의 발전 과정에 대한 이해를 공유하는 기회가 생김
  • 리뷰어는 작성자의 코드를 자세히 보면서 유용한 트릭을 배우거나, 작성자에게 유용한 트릭을 알려줄 기회를 발견할 수 있음
• 그러나 이는 '라이트 사이드' 코드 리뷰어들이 사용하는 방식임. 코드 리뷰를 코드 개선과 개발자들의 집단적 기술 향상을 위해 사용하는 것임
• 코드 리뷰는 완전히 다른 목적을 위한 강력한 도구가 될 수도 있음. 리뷰어가 '다크 사이드'로 전환한다면, 코드 개선을 방해하거나 지연시킬 수 있는 다양한 방법을 사용할 수 있음
  • 패치 작성자를 괴롭히거나 완전히 좌절시키는 등 다른 개인적인 목적을 추구할 수도 있음
• 최근에 '다크 사이드'로 전환한 리뷰어라면 아직 모든 가능성을 생각해보지 않았을 수도 있음
  • 그래서 이 글에서는 다크 사이드 코드 리뷰어들을 위한 안티패턴 리스트를 제시함

관련글: https://www.chiark.greenend.org.uk/~sgtatham/quasiblog/code-review-antipatterns/

Code review antipatterns

 

[안티패턴]

The Death of a Thousand Round Trips

• 코드를 읽으면서 사소한 것을 발견하자마자 리뷰 댓글로 지적하고, 그 즉시 읽기를 중단함
• 개발자는 성실하게 그 사소한 것을 고치고 수정된 패치를 제출함
• 리뷰어는 그 버전을 읽기 시작하고, 또 다른 사소한 것을 발견함. 첫 번째 리뷰에서 언급할 수 있었지만 그러지 않았음. 그 사소한 것을 지적하고 다시 읽기를 중단함
• 이를 개발자가 희망을 잃을 때까지 반복함

The Ransom Note

• 이 패치가 개발자에게 특별히 중요한 것 같음
• 그러나 리뷰어에게는 그렇게 중요하지 않음. 따라서 리뷰어는 권력 위치에 있음
• 개발자가 원하는 변경 사항을 리뷰어에게 중요한 추가 작업들이 완료될 때까지 인질로 잡아둘 수 있음

The Double Team

• 하나의 패치, 두 명의 리뷰어
• 한 리뷰어가 변경을 요구할 때마다 개발자는 순순히 변경함. 그러면 다른 리뷰어가 불평할 차례가 됨
• 리뷰어들은 번갈아가며 서로 상충되는 요구사항을 제시함
• 개발자가 포기할 때까지 리뷰어들이 개발자를 앞뒤로 몇 번이나 튕겨낼 수 있는지 보는 것임

The Guessing Game

• 문제가 있고, 그 문제를 해결할 수 있는 다양한 방법이 있음
• 개발자는 한 가지 해결책을 선택하고 패치를 제출함
• 리뷰어는 그 특정 솔루션을 문제 해결 여부와 무관한 이유로 비판함
• 비판을 충분히 모호하게 하면 개발자는 기존 패치를 어떻게 수정해야 비판을 해결할 수 있을지 알 수 없게 됨
• 그러면 리뷰어는 다시 마찬가지로 도움이 되지 않는 방식으로 그것을 거부함

The Priority Inversion

• 첫 번째 코드 리뷰에서는 사소하고 간단한 것들을 지적함
• 개발자가 그것들을 수정하기를 기다렸다가 중대한 문제를 제기함
• 이것만으로도 개발자가 포기하기에 충분할 수 있음

The Late-Breaking Design Review

• 몇 주 또는 몇 달 동안 많은 별도 패치로 엄청나게 복잡한 작업이 진행되어 왔음
• 리뷰어는 그 전체 작업의 설계에 동의하지 않지만, 원래 논의에는 참여하지 않았거나 토론에서 패배한 측이었음
• 그런데 이제 리뷰어에게 그 작업의 중간에 있는 사소하지만 중요한 부분(예: 많은 개발자를 막고 있는 버그에 대한 쉬운 수정)을 검토해달라는 요청이 들어옴. 이것이 리뷰어에게는 기회임
• 리뷰어는 지금까지 수행된 작업의 전체 설계에 대한 정당성을 요구함
• 개발자가 전체 작업의 일부만 담당하고 있어서 답변을 모른다면, 그것은 리뷰어의 문제가 아님. 리뷰어가 만족할 때까지 "승인" 버튼을 누르지 않을 것임

The Catch-22

• 하나의 큰 패치라면 읽기가 너무 어려움. Self-Contained된 하위 조각으로 분할할 것을 요구함
• 반대로 작은 패치가 많다면 그 중 일부는 그 자체로는 의미가 없음. 다시 붙여넣을 것을 요구함
• 어떤 종류의 트레이드오프가 특정 경우에 관련이 있는 것처럼 보인다면 이를 활용해 불평할 이유를 찾을 수 있음

The Flip Flop

• 코드의 동일한 부분에 사람들이 일반적으로 수행하는 유형의 변경 사항이 있음
• 리뷰어는 이전에 이러한 변경 사항을 여러 번 검토한 적이 있음
• 또 다른 변경 사항이 들어옴. 작성자는 이전 변경 사항을 자세히 살펴보고 기존 패턴을 주의 깊게 따랐으며, 이 영역에 익숙해 보이는 리뷰어를 선택함
• 리뷰어는 이전에는 전혀 문제 삼지 않았던 변경 사항의 한 측면에 갑자기 이의를 제기함. 기존 패턴을 따르는 것만으로는 충분하지 않음
• 작성자가 이전에 동일한 변경 사항을 보여주면서 리뷰어의 비일관성을 지적하면 어떻게 될까?

그러나 진지하게 ...

• 지금까지 이 글은 농담이었음. 리뷰어들이 일부러 이런 나쁜 행동을 한다고 제안하려는 것도 아님
• 실제로 이런 일을 하지 말라는 것임!
• 근본적인 문제는 권한의 남용임
• 패치 제출자가 과거에 코드 리뷰어였을 때 이런 나쁜 행동을 했다면 혐오가 정당화될 수도 있음. 그래서 코드 리뷰어의 권한을 신중하게 사용해야 함

Gatekeeping 코드 리뷰

• 여기까지는 피어 간 코드 리뷰에 중점을 두었음
• 피어 리뷰 상황에서 코드 리뷰어와 작성자는 기본적으로 같은 목표를 가져야 함
• 하지만 다른 종류의 코드 리뷰 상황에서는 그렇지 않음. 특히 프로젝트 외부인이 요청받지 않은 패치를 보내는 경우엔 매우 다름
• 하지만 다른 상황에서도 발생할 수 있음
• 이런 상황에서는 패치 수신자가 애초에 해당 변경을 원하지 않거나 수행 방식에 완전히 동의하지 않아 패치를 전혀 받아들이지 않을 가능성이 훨씬 큼
• 코드 리뷰어가 이런 '게이트키핑' 역할을 할 때는 패치가 기존 일반 설계 원칙이나 요구사항을 위반한다는 이유로 패치를 비판하는 것이 항상 잘못된 것은 아님
• 그러나 게이트키핑 맥락에서도 설명 없이 'The Guessing Game'을 적용하는 것은 무례함
• 물론 'The Death of a Thousand Round Trips' 같은 소극적이고 공격적인 방해에 대한 변명의 여지가 없음

Disclaimer

• 나는 수년 동안 내가 참여한(양쪽 모두에서) 코드 리뷰, 다른 사람들 사이에서 관찰한 코드 리뷰, 대화에서만 들은 코드 리뷰에서 이 기사를 위한 메모를 수집해 왔음
• 그래서 최근에 내 코드를 리뷰한 특정인을 겨냥한 것이 아님