コードレビューガイドラインを策定して継続的インテグレーションを実現する

こんにちは。Yahoo!広告 ディスプレイ広告エンジニアの下村です。広告配信システムの開発を担当しています。

ヤフーの広告システムは巨大なトラフィックを捌いていることやステークホルダーが多いことから、小さなバグも大きな事故になり得ます。加えて、目まぐるしく変わる法律やステークホルダーの方々からいただいた要望への対応を迅速に行うため、日々の開発速度も高い水準が求められます。しかし、以前は一度のPull Request（以下PR）に含まれる変更量が多く、レビューの時間が長引いて機能開発に時間がかかったり、大規模なコンフリクトが発生してバグが混入しやすい状態になっていました。そこで、継続的インテーグレションのプラクティスの適用を目指したところ、1週間程度かかっていたレビュー速度が大幅に向上し、当日中のレビュー完了も増加しました。

本記事では継続的インテグレーションを目指して、自分たちのチームを分析したプロセスと、工夫についてご紹介いたします。

チームの開発プロセス

まずはチームの状況をイメージしていただくために、チーム内の開発プロセスについて共有いたします。

ブランチ戦略としてGitHub Flowを採用しています。機能ブランチをメインラインから作成して開発を行い、PRを作成して承認を得た後、メインラインへマージしていく手法です。PRには自動テストや整形ツールによるフォーマットのチェックを、CIツールで行うように設定しています。当然オールグリーンでないとPRをマージできません。デプロイフローについて本記事では説明を割愛しますが、メインラインのブランチは常にデプロイ可能な状態に保つことは意識されています。

チーム全体で明確な課題意識を共有しているということはなく、特に不満もない状態で開発を行っていた、というのが取り組みを始める前の状態です。

継続的インテグレーションとは

継続的インテグレーションは高頻度（1日に数回のペース）で変更をメインラインへマージする開発手法のことです。

自チームではメインラインは1ブランチのみ存在するようにしています。そのため、トランクベース開発と形式的には一致します。トランクベース開発における機能ブランチは「短命」であることが求められます。このようなブランチのことを Short-Lived Feature Branch と呼びます。短命であることを強制する背景には継続的インテグレーションがあります。

Martin Fowlerはマージを高頻度に行うことで、マージの難易度を下げることができると説いています（参考）。マージの難易度が下がるということは、バグ混入の可能性を減らし、開発速度を向上させます。

継続的インテグレーションを学び直す機会があり、自チームの状況と照らし合わせたことが課題を発見するきっかけになりました。

チームの課題

自チームのブランチ戦略を考えたときに継続的インテグレーションができていて然るべきだったものの、そこまでの頻度でコード変更の統合を行うことはできていませんでした。PRのサイズは大きく、大規模なコンフリクトも頻繁に発生していました。これが最初に浮かび上がった課題であり、より頻繁にマージを行えるようにすることを解決策として考えました。

高頻度のマージを行うと決めたからといって開発速度が数倍になったりはしないので、継続的インテグレーションを実施するためには、マージを行う単位を、開発者が細かく分ける必要があります。しかし、機能改修を行う際にコード変更を独立したパーツに分けることは、言うが易し行うが難しであったため、タスク分割への負担や教育が継続的インテグレーションの障壁として存在しました。

仮に問題なく開発者がタスクを分割できるようになったとして、継続的インテグレーションの次の壁となるのはレビュー速度です。これは自明ですが、レビューがいつまでも行われなければいつまでもマージできません。

当時のチームのレビュー速度を自分は問題視するほど遅くもなく、誇れるほど速くもないと感じていました。具体的には2日から1週間程度でレビュー完了となることが多かったのですが、このレビュー速度は継続的インテグレーションを保つには十分な速度ではありませんでした。

継続的インテグレーションまでの道のり

タスクを分割するとき、どれだけ綺麗に分割しても完全に独立にはならず、タスクに依存関係ができることの方が多いです。そのため、分割したタスクのレビューを待っている間に、次のタスクに手をつけても、レビューの結果次第では次のタスクの途中のタスクがそのまま手戻りになってしまうことがあります。これは分割したタスクのレビュー待ちの時間は次のタスクを進めることが非効率であるということです。レビューの速度が十分に速くない場合、この待ち時間を発生させないようにタスクを分割しない方が、開発者の視点で全体の業務が早く片付くと感じるようになります。先述の通り、タスク分割それ自体が容易な作業でないこともこれを助長します。徐々に開発者が一つのPRに複数の変更を詰め込むようになると、その分さらにレビュー速度は低下します。これは正のフィードバックのため、巨大なPRと長大なレビュー時間を生まれ、継続的インテグレーションが破綻します。

よって、

• PRを分割する
• レビュー速度を上げる

この二つが同時に達成されなければ継続的インテグレーションは保てません。

上記を達成するための短絡的な方法として、「最速で最高のレビューをしろ」とプレッシャーがかけられた場合を想像してみてください。要求されるものの抽象度と水準が大きいので、かなりのストレスを感じるのではないでしょうか。あるいは、理想論と捉えてしまって実際の行動は何も変わらないと言うこともありえます。よって、単に継続的インテグレーションを自チームでも適用しようと提言するだけでは不十分だと考えました。

再度整理すると、PRを分割するためにはレビュー速度の改善が必要で、レビュー速度の改善のためにはPRを分割することが必要です。まずはより難しそうな、レビュー速度の改善から行うことが妥当であると捉え、改善に取り組むことを決めました。

レビュー速度の問題分析

レビューがなぜ十分な速度でないか考えるために、レビューにかかる時間を分割します。

まずはレビュイーの視点でレビュー時にかかる時間を考えると以下の2つの時間があります。

1. レビュワーのレスポンスを待つ時間
2. コードの修正や説明をする時間

次にレビュワーの視点で考えると、以下の3つの時間があります。

1. レビュー依頼が来てからレビューを始めるまでの時間
2. コードを理解するまでの時間
3. コードの品質が十分であるか判断するまでの時間

ここからの議論についてはGoogleのCode Review Developer Guideに大きく影響されているので、先んじてご紹介させていただきます。

さて、タスク分割を行うかどうかを判断するのはレビュイーであるため、レビュイー視点での時間が重要になります。タスク分割の観点ですとコードの修正や説明をする時間は分割の有無にかかわらず必要になるので大きく影響しません。よって、レスポンスを待つ時間を短くすることが必要であるとわかります。

定量的にレビュー時間を測っている開発者はかなり稀な存在だと思いますので、主観的な体感の時間によって判断が行われることになります。これを考慮して時間を区別する際にレビュワーのレビューを待つ時間ではなく、レビュワーのレスポンスを待つ時間としました。レビューを待つ時間とレスポンスを待つ時間は等価ではありません。レスポンスはレビューを完了させなくても取れる行動です。このことを念頭にレビュワーの行うべきアクションについて考察していきます。

すぐに着手できない

レビュー依頼は割り込みで入ってくるタスクです。依頼が来るたびにレビューを優先していては自分の業務が進まないと感じるのは自然なことでしょう。一方で、レビューを行うまでの時間はそのままレビュー時間を引き伸ばすことにつながるのでバランスが重要になります。当時のチームの状況としてレビューの優先度をもっと引き上げる必要があったことは確かですが、レビューを始める前の工夫として取り組めることがありました。最初のレスポンスを迅速に行うことです。PRに対して、レビューを行う旨を伝えたり、いつやるか宣言したり、スタンプを付与したりと、何かしらのリアクションを取ることでレビュイーのストレスは大幅に軽減され、レビューの体感時間が短くなることがチーム内の意見としてわかりました。レスポンスを行う作業はレビューを行うことに対して遥かに負荷の小さいアクションであり、割り込みタスクとしても優先度を上げることが難しくないと判断しました。よって、最初のレスポンスを迅速に行うことをチームのルールとして定めることに決めました。

すぐに理解できない

次にコードを理解するまでの時間についてですが、これは開発者の能力に依存するのでいきなり速くすることは難しいです。一方で、コードの理解に時間がかかりそうな場合にもレスポンスを迅速に行うことは可能です。正直に時間がかかりそうである旨をレビュイーへ伝えると良いでしょう。同時にいつまでにレビューが完了させるかの情報を伝えられるとベターです。

別の観点として、コードの理解に時間がかかる場合は、PRのサイズが大きすぎたり、コードの可読性が低いという可能性もあります。前者の場合は（迅速なレスポンスで）PRの分割を申し込みたいところです。最終的なゴールは適切に小さいPRを高速にレビューすることですので、PRのサイズを分割してもらうような依頼は推奨されるべきでしょう。コードの可読性については、複雑すぎるコードであったり、テストやドキュメントが不足していることが原因です。大局的にそれらが判断できる場合は細部をレビューする前にコメントをすることで、素早くフィードバックを行うことができます。

品質判断がバラバラ

最後にコードの品質の判断です。当時のチームでは完全に個人の判断に依存していました。これはレビュー観点が漏れていたり、逆に細かすぎるということを招いていました。前者はコードの品質が担保できておらず、後者はいたずらにレビュー時間を引き伸ばしてしまいます。よって、個人の基準ではなく共有された基準によって、コード品質の判断ができるようになることが必要であると考えました。

コードレビューガイドラインの設計方針

以上をまとめると、チームに必要だったものは下記3点でした。

• レビュー依頼への対応の優先度を上げる
• レスポンスを速く高頻度に行う
• コード品質の基準を共有する

これらはチームとしては約束事であるためドキュメントへまとめ、運用すると良さそうということで、コードレビューガイドラインを策定することに決めました。

コードレビューガイドラインの策定

これをふまえ、このようなイメージのガイドラインができあがりました。

レビュー依頼への対応の優先度とレスポンス速度については簡単なルールのため、ガイドラインの大部分はコード品質の基準についての記載です。

1. コードレビューの目的

まずはコードレビューの目的を明確にしました。

内容を端的にまとめると「コードベースの状態をより良くすること」としています。「よりよく」という部分が重要で、一度のPRで「完璧」や「最高」にする必要はないと決めました。

これを軸にして後続のルールについて決めていきました。

2. マージの基準

マージの基準は開発プロセスの話であり、コードレビューのルールと厳密には違うと思いますが、ガイドライン内でマージの基準を決めました。

自チームでは基本的に2つの承認が得られた場合にマージをして良いとしていますが、周辺ツールの修正やドキュメントの軽微な修正などでは1つの承認でも問題ないという慣習がありました。ここについて1つで良いのか、2つ必要なのかという不毛な困惑を避けるため、例外的に1つの承認で問題ない場合について明記しました。

3. 承認の基準

最も重要な承認の基準です。

目的に沿う形で、コード修正によってコードの全体的な品質が改善するとわかれば承認することが大原則となっています。

上述の品質の考え方として、優先順位をつけています。具体的には下記の優先度でコードの選択が行われます。

1. 一般的なベストプラクティスやデータ、スタイルガイドに基づいた決定
2. コードベースとの一貫性
3. 開発者の意見と好み

ここで、スタイルガイドという新しいガイドラインについて言及されています。スタイルガイドはコードの見た目に関するものだけではなく、ループの書き方や、型の優先順などプログラムの書き方を規定するものです。GoogleのC++スタイルガイドなどが参考になると思います。

コードベースとの一貫性が開発者の趣向よりも優先度が高いことも特筆すべき点かもしれません。裏を返すと、最後の判断基準は開発者の意見と好みであり、スタイルガイドに記載がなくコードの一貫性も損なわない場合は開発者が書いたコードを尊重することが求められます。レビュワーは直感的にベターだと思ったコードを提示するだけではコードの変更依頼を行うことはできず、なぜ変更したほうが良いのかをしっかりと説明する必要が生じました。このことで、コードレビューの納得感が向上し、レビュワーの個人的な趣向を押し付けることも減りました。

4. レビューコメントの基準

「承認の基準を満たしているものの、もっと改善できるのに…」と思うことも多々あります。

そのような場合に学習の機会を奪わず、きちんとコメントを残すようにレビューコメントの基準も記載しています。

具体的には承認をした場合でも、さらに改善できる部分にはコメントを残すような記載と、後述するラベルをつけて対応が必須でないことを示すこととしています。対応必須でないことが明記されていることで、細かい指摘であったとしてもレビュイーにかかるストレスは小さくなりました。

5. コードレビューのスピード

まずはコードレビューの速度がチームの生産性に直接影響する旨を記載しています。これはレビューの優先度を上げることを意図しています。

次にレスポンス速度についてもルールを決めています。

• 最初のレスポンスは1営業日以内に行うこと
  • 「本日時間が取れないので、金曜日までにはレビューします」とかでもOK

最後にチーム内で暗黙的に行われていたベストプラクティスをルールとして明記しました。

• コード変更が大きすぎるとレビュワーが判断した場合は分割を依頼すること
• 議論が活性化された時や、指摘事項が多い時は同期レビューを行うこと
• レビュイーはレスポンスが来ない場合や修正依頼への対応が完了した時はレビュワーへレビュー要請を行うこと

ルールとなることで、遵守すべきものへ昇華されるので、例えばレビュー要請など心理的ハードルの大きくなりがちな行動についても、チームの文化として根付かせることができました。

6. レビューコメントのフォーマット

[優先度観点ラベル][レビュー観点]

内容

(サンプルコード)

(参考資料)

といったフォーマットを用意しています。例を示します。

[imo][複雑性]

並行に処理を行うように実装していますが、ボトルネックになるような部分ではないため、可読性を優先させて同期処理にしても良いかと思いましたがいかがでしょうか？

優先度観点ラベルは

• must
• ask
• imo
• nits
• good/like
• memo

を用意しています。上の二つ「must」「ask」を付与したコメントが解決しないうちは承認を行ってはいけません。逆に、「must」「ask」がない場合は承認を行えるコードということになるので、いたずらに承認を妨げてもいけません。優先度を明記することで、レビュイーにとって承認までに必要な対応が明確になりました。

最後にレビュー観点についても列挙しています。こちらについてはGoogleのレビュー観点を参考にしています。

成果

レビュー速度について、ほぼ1日、遅くとも2〜3日で終わるように徐々になっていきました。特に、当日中にレビューが完了することが大幅に増えたことは大きな成果です。タスクの分割についてはうまくいかない時も多く、そのような場合にはレビューが比較的遅くなってしまうという問題は残っており、まだまだ改善の余地があります。

コードレビューガイドラインを策定したそのほかの効果として、心理的安全性が高くなったことが挙げられます。細かい改善点などのレビューは引き続き行っていますが、優先度観点ラベルや、マージまでの基準が明確化されたことで、コメントの意図や温度感が同時に伝えられるようになったので、コメントを攻撃と感じたり、先輩に質問を行うかどうか気を揉んだりすることが減ったという意見がありました。

まとめ

本記事では継続的インテグレーションの理想的な状態とチームの状況を見比べ、チームの改善点や課題点を炙り出した後、解決策としてコードレビューガイドラインを策定した取り組みをご紹介いたしました。

策定にそれほど時間はかからずに効果は大きい取り組みだと感じたので、ぜひお試しください。