「巨大プルリク1件vs細かいプルリク100件」問題を考える(翻訳)

概要

原著者の許諾を得て翻訳・公開いたします。

「巨大プルリク1件vs細かいプルリク100件」問題を考える(翻訳)

本記事では、昔ながらの問題である「巨大なプルリク1件と超細かいプルリク100件、どっちなら戦う気になれる?」に対する回答を示したいと思います。チームの一員としてよりよいコードを書くためのガイドラインについてもある程度解説します。今回の記事は、すべて以下のツイートから触発されました。

何が問題だったか

私は、Fullscript社で行われているコードレビューが今ひとつ活用されていないことに気づきました。featureブランチが長期間取り残されていることがしょっちゅうでした。featureブランチのコードは数千行にまで肥大化し、まともなフィードバックを返すことはおろか、レビューに途方もない時間がかかる始末です。心底ゲンナリでした。

開発者は、時間とやる気が満ちてくるまでレビューを先延ばしにしたりするので、開発プロセスが停滞してしまうことがあります。レビュアーは「レビューしなければ」というプレッシャーを肌で感じつつ、コードの表面をさっと眺めて「LGTM!👍」(良さげ)などと書いて終わらせることもよくあります。

たとえレビューがうまくいったとしても、大規模なリファクタリングをかけるにはタイミング的に手遅れになることもしばしばです。初期段階の設計ミスが頑固に根を張り、修正コストはスタートアップ企業が到底負担しきれないほどに跳ね上がってしまいます。ぐらついている基礎の上で何週間も作業を重ねたこともありました。

残念なことに、コードの品質は詳細な検査が必要なレベルにすら達しませんでした。誰もそこから学んでおらず、レビュープロセスは頓挫してしまったのです。

ほほう、ではどうやって改善する?

レビューを依頼するということは、他の誰かに「責任を共有してください🙇」とお願いするということです。依頼された人は問題を理解し、あなたのコードを把握し、問題がなければ(そう願いたいものです)承認しなければなりません。私たちは開発者として、この作業をできる限り軽減すべきです。

「早期」かつ「頻繁に」

フィードバックを依頼するのは、最も重要な時期、すなわち開発プロセスの早い段階で行うようにしましょう。こうすることで、レビュアーは設計上の問題を早い段階で検出する機会を得られますし、あなたが確かな基礎の上にコードを構築していることを担保できるようになります。チームは、開発プロセスが進んでからの書き直しというコストの高い作業や、既知の欠陥を持つコードを時間や予算の制約のせいでそのままmergeする事態を回避できます。

粒度を小さくする

「小さく」というのは、限りなくゼロ行に近づけるということです。たった1行の変更のレビューを嫌がるレビュアーはいないでしょう。プルリクのサイズ(=コードの行数)に上限を設けることで、粒度を下げやすくする効果が著しく向上します。レビュアーが1行ずつ丁寧に調べやすくなるのはもちろんのこと、レビュー時間も大きく削減できます。コードを定期的にmergeできるようになり、品質にも自信を持てるようになります。

作業のスコープを絞る

コードの行数を削減するための重要なコツは、プルリクのスコープ(=機能のセット)を絞り込み、解決するタスクを1つにする(または密接に関連する少数のタスクに絞り込む)ことです。スコープを絞ることで、レビュアーの認知機能にかけられる負荷を大きく軽減できます。1件のプルリクでいくつもの問題をいっぺんに解決しようとすると、ある問題がどのコードと関連しているのかを整理する作業がつらくなります。

機能が未完成でもリリースする(ただし内緒で)

機能が完成するまでリリースを差し止めることは比較的普通に行われます。残念なことに、これは巨大なfeatureブランチがいつまで経ってもなくならない主要な原因のひとつです。masterブランチでの開発が進むに連れて、mergeのコンフリクトやrebaseなどの楽しい事件が起きがちです。たとえmergeできたとしても、大規模な変更を無事にデプロイするのはチームにとって神経を削る作業です。目玉機能や大きな依存関係のアップグレードをデプロイする場合はなおさらです。

機能を一時的に取り消すツールを使って、未完成の機能をユーザーの目から隠しておくという手があります。これなら、コードのmergeやリリースの頻度が落ちずに済みますし、心配の種も減らせます。開発が完了に近づいたら、特定のアカウントやアーリーアダプタ(訳注: 新機能を喜んで使うユーザー)にだけ新機能へのアクセスをぼちぼち許可できるようになります。デプロイ作業の心配も減りますし、機能の成熟度に応じて機能へのアクセスを制御できるようになります。

追伸: Ruby on Railsをお使いの方には、flipper gemを強くおすすめいたします。

事前の計画

開発者がこうした制約のもとで作業すると、問題を細かな単位に分割して渡すようになります。この制約は、そのような明確な計画のない開発を手がけようとする誘惑を払いのけるのにも役立ちます。これには少々経験が必要ですが、そのうちに慣れて、自然に開発プロセスの一部に組み込まれるでしょう。

「ついでのリファクタリング」はしないこと

開発者は、問題に気づくとその場でリファクタリングすることがよくあります。リファクタリングはよいことですが、別のプルリクで行うべきです。そうすることでリファクタリングを早めにmergeできますし、関係のない機能リリースに修正が結び付けられることもなくなります。「ついでの改善」は、元々のプルリクのリリースが遅れれば巻き添えで遅れてしまいますし、最悪まったくリリースされなければそのまま失われてしまうでしょう。

やってみよう

コードレビューは、チーム内でのソフトウェア作成になくてはならない作業です。コードレビューは知識を共有する場であり、コードの品質を監視する門番でもあります。上述の制約の元で作業するようになったことで、Fullscript社のコードレビューで次のような成果を得られました。

  • コードレビューが短時間で完了し、品質も向上した
  • リリースのテストが容易になり、デプロイの苦労も大きく軽減された
  • 問題が発生した場合の変更の取り消しやロールバックが簡単になった

単純な話のように見えるかもしれません(し、実際そうかもしれません)が、実践のためにはチームが一丸となって経験を積む必要があります。本記事でお伝えしているメッセージに共感いただけましたら、始め方についてチームメンバーと話してみることをおすすめします。


ご意見や疑問がありましたら、元記事のコメント欄かTwitterまでお気軽にどうぞ。

また、本記事は私が地元のミートアップで発表したスピーチを元にしています。このスピーチは元々、大規模チームでのソフトウェア開発のアプローチについてKevin McPhillipsWillem van Bergenと雑談したときに閃いたものです。お二人に感謝します。

関連記事

いい感じのコードには速攻でLGTM画像を貼ってあげよう

Rails: テストのリファクタリングでアプリ設計を改良する(翻訳)

技術的負債を調査する10のポイント(翻訳)

Ruby on RailsによるWEBシステム開発、Android/iPhoneアプリ開発、電子書籍配信のことならお任せください この記事を書いた人と働こう! Ruby on Rails の開発なら実績豊富なBPS

この記事の著者

hachi8833

Twitter: @hachi8833、GitHub: @hachi8833 コボラー、ITコンサル、ローカライズ業界、Rails開発を経てTechRachoの編集・記事作成を担当。 これまでにRuby on Rails チュートリアル第2版の半分ほど、Railsガイドの初期翻訳ではほぼすべてを翻訳。その後も折に触れてそれぞれ一部を翻訳。 かと思うと、正規表現の粋を尽くした日本語エラーチェックサービス enno.jpを運営。 実は最近Go言語が好き。 仕事に関係ないすっとこブログ「あけてくれ」は2000年頃から多少の中断をはさんで継続、現在はnote.muに移転。

hachi8833の書いた記事

週刊Railsウォッチ

インフラ

BigBinary記事より

ActiveSupport探訪シリーズ