プルリクエストの単位を小さくし、コードレビューの負荷とマージまでの期間を大幅短縮した話

これは アソビュー!Advent Calendar 2023のB面 19日目です。 🎄 今年のアドベントカレンダーは2面公開なので、ぜひそちらも御覧ください!

こんにちは、バックエンドエンジニアのアズマです。 今回は、プロダクトコードの品質に関わる話の1つで、プルリクエストの単位を小さくし、そのレビューにかかる負荷と、マージまでの期間を大幅に短縮した話をします。

弊社のタスクとコード管理

弊社では開発スプリントや修正タスクの管理はJiraで、コードの管理はGitHubで行っております。 新機能の開発、機能改修、不具合の修正、設定変更など、それぞれタスクの粒度は異なりますが、適宜対応するタスクにあわせてJiraのチケットを発行し、このチケットをもとにプロダクトコードを作成しています。

作成したコードは常にチーム内でGitHubのプルリクエストを作成してコードレビューを設け、レビュワーはその修正箇所を見てレビュイーに確認、コードの改善点などを相談し、静的解析だけでは判別できないところを見つけてより良いプロダクトコードを目指しながらリリースしていきます。

実際にタスクへ落としこむ際には、次の条件でタスクを分割しています。

  • 複数のプロダクトへ変更が入る
  • リリースタイミングを分割したい

この理由としては作業する私も、管理するマネージャさんたちも、進み具合と影響範囲がわかりやすいからです。分ける基準としては、私の場合は最大1週間で終わる範囲で、1つのJiraチケットを作成しています。 そしてこのJiraのチケット単位でgitのブランチを作成*1していきます。

実際に起こった課題

Jiraのチケット単位でコードを作成していきますが、その作成内容によってはコードの修正範囲や修正するファイル数が異なりますので、修正量が少ないものから、重厚長大な大作ができてしまう可能性もあります。

チケットを作成した当初では気づかなかった変更点の洗い出しの漏れが起因となって、具体的にはコードの修正範囲の多さやファイル数が当初の予想よりも多くなっていき、そしてそのまま作業を進めていくにつれ肥大化した変更量のままレビュー依頼を出し、それが元でコードレビューの妨げになってしまう事態に直面しました。

例1 ちょっと多いかな?

例えば、これくらいなら、ちょっと変更量が多いかな?と思うところです。

プルリクエストの例1。これでもちょっと多そう。

実際にレビューしていただいだ結果、少し時間をかければ見れそうなレベルとのことで許容範囲とのことでした。

例2 ファイル数が爆発

次にわたしのやらかしです。ファイル変更数が99、行数にして 追加が 2502行、削除が 40行。

さすがにこれは「ちょっと待て、これを見なきゃならんのか?」とツッコミが入ってもおかしくなさそうです。実際にはテンプレートファイルが大量にあったため、これに対応する単純なロジックも追加しているのが主な原因でした。単純な実装が多いとはいえ、その変更ファイル数にレビューする側も身構えてしまいます。

例その2。ファイル変更数が多すぎて読むのも苦しい。

ただし、わたしがこのレビュー依頼を出す前には

「ファイル数が多くなっているので、ここだけ見てほしい」

と一言添えてレビュー依頼を出していたため特に問題なかったところですが、この一言がなければそのまま全部見ることになるでしょうから*2、人間の脳にある一時バッファサイズでは非常に厳しいものがあり、かなりの集中力を要します。

例3 スマッシュ大作

以下の例は、1つの修正レビューがあまりに巨大すぎたため、レビューが停まった例です。

例3 修正箇所が多すぎて停まった例

ファイル変更数が200超え、変更行数が15,000行超え。

その修正内容の内訳を改めて見ると、対象Jiraチケットに対して修正する範囲が広範囲にわたっていたにも関わらず突き進んでしまい、修正内容が大きい状態になっているのに中間レビューや確認もほぼないままレビュー依頼が出てきたものでした。

実際にかかったレビュー期間は2週間以上で、最終的には残念ながら仕様を満たしていない可能性*3を見つけ、これをさらに改修するよりも、やり方を変えないとならないことが判明して一度取り下げる運びになりました。

ふりかえりと実施したこと

ここで私たちのチームはふりかえりを実施し、あらためて課題点の共有と、目安を作ることで、コードレビューの負荷とレビューの長期化に伴う要因を共有しました。

その結果を受け、実施した内容は以下のとおりです。

  • プルリクエストを作成したあと、必ず自分で確認する
    • 想定してなかった変更があるか
    • 消す予定だった内容が残っていないか
  • 大きな変更があるときには事前に共有する
    • 変更ファイル数が25を超えた*4場合は、変更の量が妥当かを確認する
  • レビュワーに見やすい状態であるか
    • 変更箇所がわかりにくいと思った場所は、レビュイーが事前にコメントをしておく
    • 特に見てほしい箇所には、自らプルリクエスト内にコメントを残しておく

今までは個人の気づきにだけ依存した*5コードレビュー対象となるコード量やレビュー観点ですが、より具体的にすることで、ここまでに例示した悲劇は回避されました。 これにより、以下のシナジーが生まれました。

レビュワーの負担が軽くなる

レビューの完了時間が短くなる

開発者の待ち時間や、手戻り対応の期間も短くなる!

結果としてマージまで短くなる

開発サイクルが軽くなり、プロジェクト管理やスプリント内のタスク整理がわかりやすくなった

これだけでも相当な効果がありますね!そして何よりも

「レビュワーの負担が理解できる」

ことが、チームへの貢献に大きく寄与できている点だと個人的には思います。

終わりに

アソビューでは「生きるに、遊びを。」をミッションに、一緒に働くメンバーを募集しています! ご興味がありましたら、まずはカジュアル面談からご応募いただければと思います!

www.asoview.com

*1:作業量が多くなりそうなときにはさらにブランチも分割することもありますが、今までは個人の裁量だけでした

*2:ただし本当に真面目に見てくれるなら…という性善説的なところもあります

*3:個人的には、コードレビューだけでは仕様を満たしているかの判断はつかないケースはあります。

*4:SpringBootで作成したプロジェクトにおいて、わたしたちのチームが設定したクラス数の目安です

*5:個人的にはプルリクエスト作成後に自分で内容を見直すフローがありましたが、あらためてルール化して周知することで暗黙知への不安が解消されます