Code review antipatterns

원문

코드 리뷰 안티패턴

[Simon Tatham, 2024-08-21]

소개(Introduction)

코드 리뷰는 좋은 아이디어처럼 보이죠, 그렇지 않나요? 두 명의 개발자가 같은 코드를 보면 문제를 발견할 기회가 두 배가 됩니다. 프로젝트가 어떻게 발전하고 있는지에 대한 이해를 공유할 수 있고, 리뷰어는 작성자의 코드를 자세히 읽으면서 유용한 트릭을 배울 수 있거나, 작성자가 아직 모르는 유용한 트릭을 가르칠 기회를 발견할 수 있습니다.

하지만 이는 '밝은 면'의 코드 리뷰어들, 즉 코드베이스와 개발자들의 집단적 기술을 향상시키려고 노력하는 이들에게만 해당됩니다. 코드 리뷰는 완전히 다른 목적을 위한 강력한 도구가 될 수도 있습니다. 코드 리뷰어가 어두운 면으로 돌아설 때, 그들은 코드의 개선을 방해하거나 지연시키고, 패치 작성자를 짜증나게 하거나 완전히 낙담시키거나, 또는 그들 자신의 다른 목표를 추구하는 다양한 방법을 선택할 수 있습니다.

만약 당신이 최근에 어두운 면으로 돌아섰다면, 아직 모든 가능성을 생각해 보지 않았을 수도 있습니다. 그래서 여기 어두운 면의 코드 리뷰어를 위한 코드 리뷰 안티패턴 목록을 준비했습니다. 아이디어가 떨어진 분들을 위해서요.

안티패턴들(Antipatterns)

천 번의 왕복의 죽음 (The Death of a Thousand Round Trips)

코드를 읽기 시작합니다. 사소한 지적거리를 발견하자마자, 그것을 지적하는 리뷰 코멘트를 추가합니다. 그리고 읽기를 멈춥니다.

개발자는 성실하게 당신의 지적을 수정하고, 수정된 패치를 제출합니다.

당신은 그 버전을 읽기 시작합니다. 첫 번째와는 독립적인 또 다른 사소한 문제를 발견합니다. 처음에 충분히 언급할 수 있었지만, 그때는 거기까지 읽지 않았기 때문에 언급하지 않았습니다. 그래서 이번에는 그것을 지적합니다 - 그리고 다시 읽기를 멈춥니다.

이런 식으로 계속됩니다. 개발자가 희망을 잃을 때까지 반복합니다.

만약 당신이 개발자와 매우 다른 시간대에 있다면, 자연스럽게 24시간의 왕복 시간이 생겨서 패치가 가능한 한 천천히 진화하게 됩니다. 만약 같거나 가까운 시간대에 있다면, 패치를 적절히 지연시키기 위해 다른 여러 가지 일로 바쁘다고 핑계를 대야 할 것입니다. 그래야 각 새 버전을 살펴보는 데 하루나 이틀이 걸린다는 좋은 변명이 됩니다.

"이제 많이 좋아졌네요"와 같은 말을 하고 싶은 유혹을 참으세요. 만족하고 있다는 힌트를 주면, 그들에게 계속 노력할 이유를 줄 수 있습니다!

몸값 쪽지 (The Ransom Note)

이 특정 패치가 제출한 개발자에게 특별히 중요해 보입니다. (아마도 그들이 패치를 받아들이도록 설득하려는 과정에서 직접 그렇게 말했을 수도 있고, 아니면 당신이 그냥 행간을 읽은 것일 수도 있습니다.)

하지만 이 패치는 당신에게 특별히 중요하지 않습니다 - 그래서 당신은 권력의 위치에 있습니다! 이제 그들이 필요로 하는 변경사항을 인질로 잡고, 그들에게 많은 추가적인 관련 작업을 하도록 할 수 있습니다. 이 작업들은 실제로 같은 커밋에 있을 필요가 없지만, 당신에게는 중요한 것들이죠.

'만약 당신의 소중한 패치를 다시 보고 싶다면...'

이중 팀 ( The Double Team)

하나의 패치. 두 명의 리뷰어.

당신들 중 한 명이 변경을 요구할 때마다, 개발자는 순순히 그것을 수행합니다 - 그리고 이제 다른 한 명이 불평할 수 있습니다!

서로 양립할 수 없는 요구를 번갈아가며 하세요. 하지만 항상 개발자에게 직접 코멘트를 하세요. 리뷰 스레드에서 서로 직접 논쟁하는 것을 피하고, 패치 작성자가 당신들 둘이 서로 이야기를 나누고 패치가 어떤 모습이어야 하는지에 대해 합의해야 한다는 제안을 인정하지 마세요.

개발자가 포기하기 전에 얼마나 많이 앞뒤로 핑퐁할 수 있는지 봐보세요.

(실제 긴급 상황에서, 만약 공모자를 찾을 수 없다면, 자신의 이전 리뷰 코멘트와 모순되게 할 수도 있습니다. 하지만 보통은 누군가가 알아차릴 겁니다. 두 명의 리뷰어가 있을 때 더 잘 작동합니다.)

추측 게임 (The Guessing Game)

문제가 있고, 그것을 해결할 수 있는 많은 다른 방법들이 있습니다. 개발자는 한 가지 해결책을 선택하여 패치를 제출했습니다.

그 특정 해결책을 비판하세요. 하지만 그것이 문제를 해결하는지 여부와는 관련 없는 근거로 비판하세요. 당신의 비판을 모호하고 불분명하게 만드세요: 어떤 모호한 설계 원칙의 위반이라든가, 혹은 같은 영역에서의 미래 개발을 위한 어떤 허무맹랑한 계획과의 비호환성 등으로요. 비판을 그들이 실제로 달성하려고 했던 것과 최대한 무관하게 만드세요.

충분히 모호하게 만들면, 개발자는 당신의 비판을 해결할 수 있다고 확신할 수 있는 방법으로 기존 패치를 수정할 방법을 생각해낼 수 없을 것입니다. 그들의 최선의 선택은 원래 문제에 대한 완전히 다른 해결책을 선택하고, 그것을 대신 구현하려고 시도하는 것입니다.

그러면 이제 당신은 그것도 똑같이 도움이 되지 않는 방식으로 무시할 수 있습니다!

개발자가 "그럼 이 문제를 어떻게 해결해야 한다고 생각하세요?"와 같은 대화로 당신을 함정에 빠뜨리지 못하게 하세요. 당신이 만족할 만한 해결책에 대한 어떤 힌트도 주지 마세요. 결국에는 그들이 계속 추측하는 것을 포기하게 될 것입니다.

우선순위 역전 (The Priority Inversion)

첫 번째 코드 리뷰 단계에서는 작고 간단한 지적을 하세요. 변수 이름이 좀 불분명하다거나, 주석에 오타가 있다거나 하는 식으로요.

개발자가 그것들을 수정하기를 기다렸다가, 그 다음에 폭탄을 떨어뜨리세요: 패치에 훨씬 더 근본적인 문제가 있어서, 일부분을 완전히 다시 작성해야 한다고 말이죠 - 이는 당신이 이미 개발자에게 시켰던 많은 사소한 수정 작업을 버리게 만드는 것을 의미합니다.

개발자의 작업이 원하지 않고, 그들의 시간을 가치 있게 여기지 않는다는 것을 보여주는 것보다 더 좋은 방법은 없습니다. 이것만으로도 개발자가 포기하게 만들기에 충분할 수 있습니다.

늦은 설계 리뷰 (The Late-Breaking Design Review)

엄청나게 복잡한 작업이 몇 주 또는 몇 달 동안 여러 개의 별도 패치로 진행되어 왔습니다. 작업의 절반이 이미 커밋되었습니다.

당신은 개인적으로 전체 작업의 설계에 동의하지 않지만, 원래 논의에서 당신의 의견을 묻지 않았습니다. 아니면 어쩌면 의견을 물었지만, 당신이 토론에서 진 쪽이었을 수도 있습니다.

하지만 이제 당신은 그 중간에 있는 사소하지만 중요한 것, 예를 들어 많은 개발자들을 막고 있는 버그에 대한 쉬운 수정과 같은 것을 리뷰해달라는 요청을 받았습니다. 이것이 당신의 기회입니다!

지금까지의 작업 전체 설계에 대한 정당화를 요구하세요. 만약 개발자가 대답을 모른다면, 그들이 전체 작업의 작은 부분만 담당하고 있기 때문이라고 해도 어깨를 으쓱하세요 - 그건 당신의 문제가 아니며, 당신이 만족할 때까지 '승인' 버튼을 누르지 않을 것입니다.

운이 정말 좋다면, 어쩌면 당신은 이 개발자를 조종하여 이미 완료된 작업의 일부를 되돌리게 할 수도 있습니다. 그것이 필요한 이유에 대해 그럴듯한 변명을 제공하면서 말이죠. 당신의 말을 반박하는 설계 논의를 어디서 찾을 수 있는지 알려주지 않도록 조심하세요.

22번의 함정 (The Catch-22)

만약 하나의 큰 패치가 있다면, 그건 읽기가 너무 어렵습니다! 그것을 자체적으로 완결된 하위 부분들로 나누라고 요구하세요.

반면에, 만약 작은 패치들이 많다면, 그 중 일부는 그 자체로는 의미가 없습니다! 그러니 그것들을 다시 붙이라고 주장하세요.

충분히 큰 작업이라면 당신은 분명히 이 불평 중 하나를 할 이유를 찾을 수 있을 것입니다. 어떤 것을 선택할지는 결정하기만 하면 됩니다.

꼭 '풀 리퀘스트의 패치 수'일 필요는 없습니다. 특정 경우에 관련 있어 보이는 모든 종류의 트레이드오프로 이것을 할 수 있습니다. 예를 들어, 코드가 읽기 쉽게 작성되었다면, 아마도 성능이 받아들일 수 없을 정도로 나쁠 것입니다 - 또는 잘 최적화되었다면, 읽을 수 없고 유지보수하기 어려울 것입니다.

뒤집기 (The Flip Flop)

같은 부분의 코드에 대해 사람들이 흔히 하는 변경 유형이 있습니다. 예를 들어 목록에 추가 항목을 넣는 것과 같은 것이죠. 당신은 이전에 이런 변경사항들을 여러 번 리뷰한 적이 있습니다. 또 다른 변경사항이 막 들어왔습니다: 작성자는 이전 변경사항들을 살펴보고, 주의 깊게 기존 패턴을 따랐으며, 당신을 코드 리뷰어로 선택했습니다. 왜냐하면 당신이 분명히 이 영역에 익숙하기 때문입니다.

갑자기 이전에는 전혀 문제 삼지 않았던 변경사항의 한 측면에 이의를 제기함으로써 모두를 긴장시키세요. 기존 패턴을 따르는 것만으로는 충분하지 않습니다! 작성자는 당신이 최근에 이런 유형의 변경사항이 어떤 모습이어야 하는지에 대해 마음을 바꿨다는 것을 예상했어야 합니다.

만약 작성자가 이전의 동일한 세 가지 변경사항에서 당신이 이 이의를 제기하지 않았다는 것을 보여줌으로써 당신의 불일치를 지적한다면 어떻게 해야 할까요?

그러면 이렇게 말하세요: "맞아요, 그것들도 변경되어야 해요." 개인적인 책임을 지지 않도록 주의하세요 - 당신은 기존의 모든 경우를 변경하겠다고 자원하는 것이 아닙니다. 운이 정말 좋다면 개발자가 이를 기존 케이스를 직접 변경하라는 지시로 해석할 것이고, 그러면 당신은 많은 노력을 절약할 수 있습니다.

하지만 진지하게... (But seriously, folks …)

여기까지 이 글은 농담이었습니다.

(아마 이런 말을 할 필요도 없겠지만, 누군가는 오해할 수 있으니 말씀드립니다.)

저는 실제로 코드 리뷰어들이 성가시고 방해가 되는 행동을 하도록 권장하려는 것이 아닙니다. 그리고 심지어 코드 리뷰어들이 실제로 이런 나쁜 일들을 한다고 제안하는 것도 아닙니다. 적어도 대부분의 경우에는 의도적으로 그러지 않습니다. 위의 설명들 대부분은 리뷰어가 실제로 하는 일에 대한 과장입니다. 아니면 그렇지도 않고, 그저 좌절한 패치 제출자의 입장에서 느끼는 것에 대한 과장일 뿐입니다. 패치가 몇 주 동안 리뷰에 막혀 있고, 전혀 가까워지고 있지 않다고 느껴질 때 말이죠.

제가 정말로 말하고 싶은 것은: 이런 일들을 하지 마세요! 왕복을 최대화하는 대신 최소화하려고 노력하세요. 모든 사소한 지적을 하기 전에 중요한 재작성(필요한 경우)을 요청하세요. 패치를 비판할 때는 어떤 버전을 수용할 것인지에 대해 건설적인 제안을 하세요. 코드베이스 전체에 대한 의견이 있다면, 리뷰를 맡은 한 개발자에게 소심하게 굴지 말고 모든 개발자와 일반적인 논의를 하세요. 그리고 만약 실수로 이런 일을 했다면, 자각하세요: 실수로 개발자의 삶을 더 어렵게 만들었다는 것을 인지하고, 사과하고, 더 도움이 되려고 노력하여 보상하세요. (실제 코드를 개선하는 데 도움을 줄 수도 있습니다. 이 패치의 수정된 버전을 직접 작성하거나, 후속 패치에서 추가적인 개선을 하는 식으로 말이죠.)

권위 (Authority)

하지만 여기서 진지한 요점을 말하자면, 이것이라고 생각합니다. 한 개발자가 다른 개발자의 패치에 대한 코드 리뷰어가 될 때, 그 관계는 일시적인 권위를 만듭니다. 리뷰어는 다른 시간에는 패치 제출자에 대한 어떤 종류의 권위도 없더라도, 그 특정 패치가 커밋되는 것을 막을 수 있는 권력을 갖게 됩니다.

권위에는 책임이 따릅니다. 당신은 그 권위를 의도된 목적으로만, 그리고 필요한 만큼만 사용해야 합니다. 이 경우, 그것은 개발 팀 전체가 합의한 '좋음'의 정의에 따라 패치를 가능한 한 좋게 만드는 것입니다.

따라서 이러한 안티패턴들(또는 그것들이 풍자하는 더 온건한 행동들) 대부분은 권위의 남용입니다. 리뷰어는 다른 개발자에 대한 이 일시적인 권력을 지렛대로 사용하여 다른 개인적인 의제를 추구합니다. 아마도 패치의 좋음과는 무관하거나, 어쩌면 적극적으로 그것에 반대되는 의제일 수도 있습니다.

이러한 안티패턴들 사이에서 개인적인 의제는 다양합니다. 리뷰어가 선호하는 관련 없는 작업일 수도 있고, 리뷰어가 팀의 나머지 구성원들에게 채택하게 하지 못한 개인적인 스타일 선호일 수도 있습니다. 원하는 것에 대한 한 줄 설명을 쓰고 다른 사람이 어려운 작업을 하게 하는 기회를 이용하는 게으름일 수도 있고, 단순히 변화에 대한 저항이거나 어쩌면 패치 제출자에 대한 개인적인 반감일 수도 있습니다.

그리고 그 반감은 패치 제출자가 마지막으로 코드 리뷰어 차례였을 때 이런 나쁜 행동들을 했다면 꽤 정당화될 수 있습니다! 이것이 코드 리뷰어의 권위를 아껴 써야 하는 또 다른 이유입니다. 만약 개발자들이 서로에 대한 복수의 순환에 빠지게 된다면, 당신의 소프트웨어 프로젝트는 운명이 다한 것입니다.

게이트키핑 코드 리뷰 (Gatekeeping code review)

지금까지 저는 동료 간 코드 리뷰에 초점을 맞추었습니다. 코드 리뷰어와 패치 제출자는 동료입니다; 누구도 다른 사람을 책임지지 않습니다; 누구도 일반적으로 코드베이스에 대한 최종 결정권을 갖지 않습니다. 그래서 코드 리뷰에서 얻는 권위가 일시적인 것입니다: 내일, 이 패치가 적용된 후에는, 더 이상 그 권위가 없을 것입니다. 그 다음날에는 상황이 뒤바뀔 수도 있습니다.

또한, 동료 리뷰 상황에서는 코드 리뷰어와 작성자가 기본적으로 같은 목표를 가지고 있어야 합니다. 어떤 기능이 코드에 들어가야 하는지, 승인 전에 얼마나 다듬어져야 하는지, 수용 가능한 코딩 스타일이 무엇인지에 대한 심각한 의견 차이가 있다면, 코드 리뷰 맥락 밖에서 처리되어야 합니다.

하지만 다른 종류의 코드 리뷰 상황에서는 그렇게 작동하지 않습니다. 특히, 소프트웨어 프로젝트 외부의 사람이 요청하지 않은 패치를 보내온다면, 그것은 매우 다릅니다.

(이 시나리오는 보통 자유 소프트웨어에서 발생합니다. 세상 누구나 당신의 소스 코드를 수정할 수 있고, 그 중 일부는 변경사항을 당신에게 다시 보내려고 할 것이기 때문입니다. 하지만 다른 상황에서도 발생할 수 있습니다 - 독점 코드를 개발하는 회사 내에서도, 한 팀의 개발자가 다른 팀의 프로젝트에 패치를 보내려고 시도할 수 있습니다. 만약 그들이 특별히 원하는 기능이나 수정사항이 있지만 다른 팀이 아직 노력을 기울이지 않았다면 말이죠.)

이런 상황에서는 패치를 받는 쪽이 그것을 전혀 받아들이지 않으려고 할 가능성이 훨씬 더 큽니다. 그 변경이 애초에 이루어져서는 안 된다고 생각하거나, 그것이 수행된 방식에 완전히 동의하지 않기 때문일 수 있습니다. 그리고 이 경우, 그것은 동료 리뷰어로서 부여된 일시적인 권위의 남용이 아닙니다: 그것은 프로젝트를 담당하는 사람 또는 팀으로서의 영구적인 권위를 정당하게 행사하는 것입니다. 이는 내 소프트웨어 프로젝트입니다 - 나는 그것이 어떤 방향으로 갈지 결정할 수 있습니다.

코드 리뷰어가 이런 '게이트키핑' 역할을 할 때, 기존의 일반적인 설계 원칙이나 요구사항을 위반한다는 이유로 패치를 비판하는 것이 항상 잘못된 것은 아닙니다 (추측 게임). 그 문제를 더 잘 해결할 수 있는 방법을 제안하지 않더라도 말입니다. 아마도 나는 그 문제를 요구사항과 일치하는 방식으로 해결할 수 있는 방법을 모를 수도 있습니다. 사실, 어쩌면 그것이 바로 어려운 부분이고, 내가 아직 같은 변경을 하지 않은 유일한 이유일 수도 있습니다!

하지만 게이트키핑 상황에서도, 설명 없이 '추측 게임'을 전개하는 것은 무례합니다. 내가 이렇게 할 때, 나는 일반적으로 개발자가 어려운 부분을 간과했다는 것을 설명하려고 노력합니다. 그리고 내가 답을 모른다면, 그렇다고 말할 것입니다.

그리고 물론, '천 번의 왕복의 죽음'과 같은 수동 공격적인 방해 행위에 대한 변명은 없습니다. 만약 당신이 진정으로 패치를 코드에 전혀 넣고 싶지 않고, 그 결정을 할 정당한 권위를 가진 게이트키핑 역할을 하고 있다면, 말로 표현할 수 있습니다. 그래서 제출자가 그것에 더 이상 시간을 낭비하지 않도록 말이죠!

유의사항 (Disclaimer)

저는 수년 동안 이 글을 위한 노트를 모아왔습니다. 제가 참여한 코드 리뷰(양쪽 모두), 다른 사람들 사이에서 관찰한 코드 리뷰, 그리고 대화에서만 들은 코드 리뷰에서 말이죠.

따라서 이는 최근에 제 코드를 리뷰한 특정 개인을 겨냥한 것이 아닙니다!