diff --git a/_posts/blog/posts/articles/2024-09-14-Code review anipatterns.md b/_posts/blog/posts/articles/2024-09-14-Code review anipatterns.md new file mode 100644 index 000000000..6d76e286e --- /dev/null +++ b/_posts/blog/posts/articles/2024-09-14-Code review anipatterns.md @@ -0,0 +1,95 @@ +--- +title: Code review anipatterns +author: 신성일 +date: 2024-09-13 19:45:24 +0900 +categories: study +tags: + - "#study" +--- +https://www.chiark.greenend.org.uk/~sgtatham/quasiblog/code-review-antipatt + +코드리뷰는 리뷰어와 개발자 모두 도움이 되는 측면이 있다. 하지만 리뷰어가 잘못하면 코드 개선에 매우 큰 장애물이 될 수도 있다. 개발자를 짜증나게하고 개선을 포기하게 할수도 있는 것이다. + +여기서는 이를 예방하기 위해 코드리뷰 안티패턴을 반어적으로 소개한다 + +## 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 + +큰 패치가 있을때 알아보기 힘드니 작게 나눠달라고 요구하라. 그런데 만약 작은 패치가 여러개 있으면 하나하나는 의미가 없으니 크게 하나로 묶어달라고 하라. + +이런 것처럼 trade-off 관계의 것을 지적하라 (ex : 가독성이 좋다면 성능을 지적, 성능이 좋다면 가독성을 지적) + +**trade-off 관계의 것을 지적할때는 조심하라** + + +## The Flip Flop + +같은 부분을 많은 사람이 수정하는 경우가 있다. (리스트 등). 개발자가 이전 관습을 준수하여 수정을 하였고, 이제까지 그러한 관습은 Approve 되었다. 그런데 갑자기 이전에 문제 삼지 않았던 부분을 문제삼아라. + +만약 개발자가 이제까지와의 불일치를 지적한다면, 이제까지의 코드도 수정해야한다고 말하라. 물론 너가 고치겠다고 자원하지마라. + + +# But seriously, folks + +물론 위 예시는 농담이고 반대로 얘기한 것이다. + +## Authority + +여기서 중요한 점은 코드 리뷰어가 개발자에 대해 권위를 가지게 된다는 점이다. 리뷰어는 코드를 승인하지 않을 권위가 생기게 된다. + +권위는 필요한만큼만 사용되어야한다. 이러한 경우 코드를 가능한 좋게 만들어야하는 걸 말하는데, 이 `좋다`는 개발팀 전체가 동의하는 정의를 따른다. + +따라서 이러한 안티패턴들은 권위를 남용하여 발생한다. 리뷰어는 코드의 장점을 무시하고 개인적인 다른 의제를 꺼내어 코드를 승인하지 않을 수 있다. 이러한 개인적 의제는 안티패턴에 따라 다양하다. + +이렇게되면 다음에 리뷰어가 개발자가 되고, 그 개발자가 리뷰어가 되었을때 복수가 시작될 수 있다. 이러한 복수의 연쇄가 되면 프로젝트는 망한다. + +## Gatekeeping code review + +따라서 필자는 peer code review 에 초점을 맞춘다. 개발팀 내부에서 서로 리뷰어가 되는 것이다. 리뷰어와 개발자는 동료이고, 이 관계는 언제든 바뀔 수 있다. peer code review 에서는 리뷰어와 개발자가 같은 목표를 가질 수 있다. + +그런데 만약 개발팀 외부의 코드를 리뷰할때는 상황이 다르다. 리뷰어는 그 코드를 승인하지 않을 가능성이 훨씬 크다. 이러한 경우는 권위를 남용한 것이 아니다. 우리 프로젝트는 우리팀에서 결정할 권한이 있기 때문이다. 이처럼 리뷰어가 게이트키퍼 역할을 할 경우, 리뷰어는 근본적인 디자인패턴을 지적할 수 있다. + +하지만 이때도 아무런 설명없이 그저 거부만하는 것은 좋지 않다 (Guessing Game 처럼). 어떻게 수정하면 좋을지 설명하고 잘 모르겠으면 모르겠다고 말하는 것이 좋다. 또한 역시 `Thousand Round Trip` 예시는 하면 안된다. diff --git a/_posts/blog/template/{{date}}-template.md b/_posts/blog/template/{{date}}-template.md index f398d10af..8acd5aa8a 100644 --- a/_posts/blog/template/{{date}}-template.md +++ b/_posts/blog/template/{{date}}-template.md @@ -1,7 +1,7 @@ --- title: template author: 신성일 -date: "{{date}}" +date: "{{date}}-({time}} +0900" categories: study tags: - "#study"