[아티클] 코드리뷰 안티 패턴들

[yeniful]

GeekNews Weekly에서 재미있게 읽은 코드리뷰 관련 아티클을 공유합니다 :-D

---

소개글

피드백을 주고받는 일은 언제나 쉽지 않습니다. 자신의 행동이나 결과물에 대해서 피드백을 받는다는 것은 두려운 일이죠. 대부분의 사람은 자신이 한 일이 옳다고 생각하며 그대로 두고 싶어 하고, 변화를 꺼리는 경향이 있습니다. 그렇기에 현재의 방식보다 더 나은 방법이 있다고 제안하는 피드백은 주는 사람에게도 부담이 됩니다. 일반적인 직장생활에서는 피드백이 1년에 한 번 또는 분기/반기별로 있는 평가에서나 이루어지지만, 개발자는 코드 리뷰라는 지속적인 피드백 과정을 매일 경험합니다. 코드 리뷰는 자신이 놓친 실수를 발견하거나, 부작용을 사전에 방지하며, 불필요한 기술 부채가 생기지 않도록 돕기 때문에 결과물을 더 나아지게 만드는 훌륭한 실천입니다. 하지만 코드 리뷰를 잘하는 것은 쉽지 않습니다. 그래서 많은 기술 블로그에서 "코드 리뷰 잘하는 법"에 대한 글을 다루며, 여러 회사가 자체적인 "코드 리뷰 가이드"를 만들기도 합니다. 반면에, 코드 리뷰 안티패턴들은 잘하는 방법이 아닌, 코드 리뷰 시 하지 말아야 할 행위들을 나열하면서 제발 이렇게만은 하지 말자고 이야기합니다. 아무리 유머라지만 보면서 뒷목 잡게 되는 글이기도 한데요. 혹시나 주변에 이런 식으로 리뷰하는 사람이 있다면, "세상에 이럴 수가" 하면서 넌지시 이 링크를 던져주세요

본문

코드 리뷰는 좋은 아이디어 같죠?

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

안티패턴

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 코드 리뷰

• 여기까지는 피어 간 코드 리뷰에 중점을 두었음
  • 코드 리뷰어와 패치 제출자는 동료이며 서로 책임지거나 코드베이스에 대해 최종 결정권을 갖지 않음
  • 그래서 코드 리뷰에서 얻는 권한은 일시적임
• 피어 리뷰 상황에서 코드 리뷰어와 작성자는 기본적으로 같은 목표를 가져야 함
  • 어떤 기능을 넣을지, 승인 전 얼마나 다듬어야 하는지, 허용 가능한 코딩 스타일은 무엇인지에 대해 심각한 의견 불일치가 있다면 코드 리뷰 맥락 밖에서 다뤄야 함
• 하지만 다른 종류의 코드 리뷰 상황에서는 그렇지 않음. 특히 프로젝트 외부인이 요청받지 않은 패치를 보내는 경우엔 매우 다름
  • 이런 시나리오는 일반적으로 프리 소프트웨어에서 발생함
  • 전 세계 누구나 소스 코드를 수정할 수 있고 일부는 변경 사항을 다시 보내려고 시도하기 때문
• 하지만 다른 상황에서도 발생할 수 있음
  • 사유(Proprietary) 코드를 개발하는 회사 내에서 한 팀의 개발자가 다른 팀의 프로젝트에 패치를 보내 운이 좋기를 바랄 수 있음
• 이런 상황에서는 패치 수신자가 애초에 해당 변경을 원하지 않거나 수행 방식에 완전히 동의하지 않아 패치를 전혀 받아들이지 않을 가능성이 훨씬 큼
  • 이 경우에는 동료 리뷰어로서 부여받은 일시적 권한의 남용이 아니라 프로젝트 책임자로서 영구적 권한을 정당하게 행사하는 것임
  • 내 소프트웨어 프로젝트의 방향은 내가 결정함
• 코드 리뷰어가 이런 '게이트키핑' 역할을 할 때는 패치가 기존 일반 설계 원칙이나 요구사항을 위반한다는 이유로 패치를 비판하는 것이 항상 잘못된 것은 아님
  • 어쩌면 요구사항과 일치하는 방식으로 해당 문제를 해결하는 방법을 모를 수도 있음
  • 사실 그것이 어려운 부분이고 내가 아직 동일한 변경을 하지 않은 유일한 이유일 수도 있음
• 그러나 게이트키핑 맥락에서도 설명 없이 'The Guessing Game'을 적용하는 것은 무례함
  • 나는 일반적으로 개발자가 어려운 부분을 간과했음을 설명하려 노력하고, 내 자신도 답을 모르면 그렇다고 말함
• 물론 'The Death of a Thousand Round Trips' 같은 소극적이고 공격적인 방해에 대한 변명의 여지가 없음
  • 정말로 패치를 코드에 전혀 넣고 싶지 않고 그 결정을 내릴 정당한 권한이 있는 게이트키핑 역할을 맡고 있다면, 제출자가 더 이상 시간을 낭비하지 않도록 말로 설명할 수 있음

Disclaimer

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

---

👀 출처
https://news.hada.io/topic?id=16472