はじめに
こんにちは、木暮です。
最近、気温が一気に落ちてきてかなり過ごし易くなりましたね！
tech.iimon.co.jp
前回はコードレビューの目的やコードレビューを円滑に進めるためのルールの作り方について復習しました。
今回はコードレビューが効果的ではなくなってしまうパターンをそれぞれ紹介し、なぜ良くないのか、ではどのように対応をすればよいのか深掘りをしていきます。
中には前回触れた内容とリンクするものもあるかと思います。
大量の変更があるプルリク
Upon reaching the pull request (PR), you find 32 files changed, 1,078 lines deleted, and 1,345 lines added—and that’s all you remember; the rest gets kind of fuzzy. You pride yourself on always giving proper, thorough reviews, but this is something else. You feel a bit
icky, but you leave the infamous “LGTM 👍” (“Looks good to me 👍”) comment and approve the PR. No one’s really going to review that many files, right?プルリクエスト（PR）を確認すると、変更ファイル32件、削除行1,078行、追加行1,345行——それだけしか覚えていない。あとはぼんやりしている。普段はきちんと徹底的なレビューを心がけているのに、今回は別物だ。少し
気持ち悪いと感じつつも、悪名高い「LGTM 👍」（「問題なさそう 👍」）コメントを残してPRを承認した。まさか誰もそんな大量のファイルをレビューしないだろう？(DeepL翻訳)引用元 Adrienne Braganza (2024) “Looks Good to Me” Constructive code reviews p274
どうしても大きい機能の開発や修正を行っているとプルリクを大きくなってしまう傾向にあると思います。
このパターンの問題点はレビュアーが確認をしなくてはいけないコードの量が多すぎて知識の伝播や品質チェックの機会を失うことです。
このLGTM症候群を防ぐためにはチーム内のプルリクを作るルールを厳格に定める必要があります。
前回の記事で触れていますが変更行数が500行、変更ファイル数は20ファイルというのが変更の上限でこの限界値で変更は小さければ小さいほど良いのです。
プルリクは意味のある最小の変更で依頼するべきです。レビュアーが多いならこの最小の単位でのプルリクを出すことはかなり効果的です。
緊急対応時のプルリク
finding a solution (quite hastily), you quickly integrate your hotfix code
straight into production, bypassing the established process. This works, and
it was warranted for the situation at hand. However, an unhealthy
precedent with your team has been established（かなり急いで）解決策を見つけ、確立されたプロセスを迂回して、
修正コードをすぐに本番環境に統合する。これは機能し、
その状況では正当化される措置だった。しかし、チームにとって
健全とは言えない前例ができてしまった(DeepL翻訳)引用元 Adrienne Braganza (2024) “Looks Good to Me” Constructive code reviews p275
急ぎの修正はどうしてもさっさと終わらせたくなる気持ちはわかります。
本番環境で起こっていることなら尚更だと思います。
これはホットフィックスとして本番環境で起きているバグ修正をしている時のケースです。
問題点はコードレビューをスキップしてしまっているという点と
一度こう言った運用がされてしまうと同じケースに直面した時にこの運用が通常フローになってしまうことです。
緊急事態であればコードレビューしなくてもいいんだという意識づけをさせないためにも
非常事態時のコードレビューのフローを明文化しルールとしてチームに浸透させる必要があります。
仲のいいメンバーにプルリクを見てもらう
The following day, you check on
the status of your PR. It’s still open. However, your other colleagues’ PRs
are all approved. You’re a bit annoyed. When you look closer, you can’t
believe it—two team members who are good friends have only been
approving each other’s PRs within minutes of being opened.翌日、あなたは自身のプルリクエスト（PR）のステータスを確認した。まだオープン状態だ。しかし、他の同僚のPRは全て承認済みだった。あなたは少し腹立たしくなった。よく見てみると、信じられない光景が——仲の良い二人のチームメンバーが、お互いのPRをオープン後数分以内に承認し合っていたのだ。(DeepL翻訳)
引用元 Adrienne Braganza (2024) “Looks Good to Me” Constructive code reviews p276
プルリクを早くマージしたい気持ちは誰にでもあると思いますが
こう言ったことをしてしまうとコードレビューが形骸化してしまい知識の伝播や品質チェックの機会を失うことや
コードレビューの公平性に欠け、こう言った行為が明るみに出た場合、チーム全体がコードレビューに対して不信感を持ってしまう点です。
チーム開発において、こういった行為をしてしまうとメンバーの間に軋轢が生じてしまう可能性もありますし絶対にやらないほうが良いと思います。
仕組みで防ぐとしたら、レビュアーは複数人とすることやレビュアーの割り当ての自動化、コードレビューはなぜ行われる必要があるのかという目的を理解してもらう啓蒙活動を行う必要があります
他のPRに依存してしまう場合
When one PR is based on another PR, which is based on another PR (and
so on, though I hope not), this is known as stacking PRs.あるPRが別のPRを基に作成され、そのPRがさらに別のPRを基にしている場合（この連鎖が続くことは望ましくないが）、これはPRの積み重ね（スタッキング）と呼ばれる。(DeepL翻訳)
引用元 Adrienne Braganza (2024) “Looks Good to Me” Constructive code reviews p280
まだ開発用ブランチにマージされていない変更が必要なパターンはよくあります。
しかしこう言ったプルリクを作ってしまった場合、レビューすべき変更内容が理解しづらい、、 依存しているプルリクに修正がある場合このプルリクも影響を受けることとなる、コンフリクトやマージ順を気にしなければいけないという問題点があり結果、コードレビューのコストが上がりレビュアーもレビュイーも管理しずらいプルリクになってしまいます。
最も良いのは開発用の親ブランチ（main,developなど）から作業用ブランチを切ることです。
しかし、どうしてもこう言ったプルリクを作る必要がある場合はプルリクにしっかりとレビューして欲しい範囲を明言することである程度はカバーできるのではないでしょうか。
未完成のプルリク
You’ve been assigned a “quick” PR to review. Lovely. You take your time,
almost reach the end of your review, and then BAM! New commits have
been pushed, meaning you need to start over.「急ぎ」のPRレビューを割り当てられた。素晴らしい。
ゆっくり時間をかけて、レビューもほぼ終わりに近づいたところで、バタン！
新しいコミットがプッシュされた。つまり、最初からやり直しだ。(DeepL翻訳)引用元 Adrienne Braganza (2024) “Looks Good to Me” Constructive code reviews p281
これはレビュー中のプルリクやレビュー完了したプルリクに対して変更が加わり何度もレビューしなくてはいけなくなってしまいます。
レビュアーからしたら、またレビューするの？どこまでレビューし直せばいいの？とすごくフラストレーションがたまることになります。
場合によってはこれもメンバー間での軋轢を生んでしまう可能性もあります。
この場合が別のプルリクで対応するか、最後まで対応してからプルリクを出す必要があります。
最後に
簡単ではありましたが、よく遭遇するコードレビューが効果的ではなくなってしまうパターンを紹介させていただきました！
コードレビューは単なるチェック作業ではありません！
小さなPRを丁寧にレビューし、チームで知識を共有することこそが、長期的に高品質なコードと健全な開発文化を作る近道だと思います！
最後まで読んでくださりありがとうございます！
この記事を読んで興味を持って下さった方がいらっしゃればカジュアルにお話させていただきたく、是非ご応募をお願いします。
iimon採用サイト / Wantedly / Green
