小さなCLs

なぜ小さなCLを書くのか?

小さくてシンプルなCLの方が以下のような利点があります:

  • 迅速にレビューされる:リビューアは小さなCLを数回に分けて5分ずつ見つける方が、1つの大きなCLを30分のブロックに割り当てるよりも簡単です。
  • 徹底的にレビューされる:大きな変更では、詳細なコメントが行ったり来たりすることで、リビューアや著者はイライラし、重要なポイントが見落とされたり落とされたりすることがあります。
  • バグを導入する可能性が低い:変更が少ないため、CLの影響を効果的に推論し、バグが導入されていないかを確認することが容易です。
  • 却下された場合の無駄な作業が少ない:巨大なCLを書いて、リビューアが全体的な方向性が間違っていると言った場合、多くの作業が無駄になります。
  • マージが容易:大きなCLで作業するのは時間がかかるため、マージする際に多くの競合が発生し、頻繁にマージする必要があります。
  • 設計が容易:小さな変更の設計とコードの品質を磨く方が、大きな変更の詳細を洗練するよりも簡単です。
  • レビューにブロックされることが少ない:全体的な変更の自己完結した部分を送信することで、現在のCLがレビューされるのを待ちながらコーディングを続けることができます。
  • ロールバックが簡単:大きなCLは、初期のCLの提出とロールバックCLの間に更新されるファイルに触れる可能性が高く、ロールバックが複雑になります(中間のCLもロールバックする必要があるかもしれません)。

なお、**リビューアはCLが大きすぎるために変更を拒否する権限を持っています。**通常、彼らはあなたの貢献に感謝し、それを一連の小さな変更にするように要求するでしょう。すでに書いた変更を分割するのは大変な作業であり、また、レビュアーが受け入れるべき理由について議論する時間がかかるかもしれません。

Smallとは何ですか?

一般的に、CLの適切なサイズは1つの自己完結した変更です。つまり、以下のような特徴を持ちます。

  • CLは、1つのことに対処する最小限の変更を行います。通常、機能全体ではなく、機能の一部に対して行われます。一般的には、CLが大きすぎるよりも小さすぎるCLを書く方が良いです。受託者と協力して、受け入れ可能なサイズを見つけましょう。
  • CLには、関連するテストコードが含まれている必要があります。
  • CLについて理解するためにリビューアが必要とする情報は、CL自体、CLの説明、既存のコードベース、または既にレビューされたCLに含まれています。
  • CLがチェックインされた後も、システムはユーザーと開発者の両方にとってうまく機能し続けます。
  • CLの影響が理解しにくいほど小さすぎるわけではありません。新しいAPIを追加する場合は、同じCL内でAPIの使用例を含めることで、リビューアがAPIの使用方法をより理解しやすくすることができます。これにより、未使用のAPIをチェックインすることも防げます。

「大きすぎる」とは具体的な基準はありませんが、通常、100行程度のCLは適切なサイズであり、1000行は通常、大きすぎるとされます。ただし、これはリビューアの判断に委ねられます。変更が広範囲にわたるファイル数も「サイズ」に影響を与えます。1つのファイルで200行の変更は問題ないかもしれませんが、50のファイルに分散されると通常、大きすぎるとされます。

自分がコードを書き始めた瞬間からコードに深く関与しているかもしれませんが、レビュアーにはその文脈がないことを念頭に置いてください。あなたにとっては受け入れ可能なサイズのCLでも、リビューアにとっては圧倒的なものに見えるかもしれません。迷った場合は、思ったよりも小さなCLを書くようにしましょう。リビューアは、あまり小さすぎるCLを受け取ったとは滅多に文句を言いません。

大規模な変更が許容される場合

大規模な変更が問題にならない場合はいくつかあります。

  • ファイル全体の削除は、リビューアがレビューするのにあまり時間がかからないため、1行の変更として数えることができます。
  • 時には、完全に信頼できる自動リファクタリングツールによって生成された大規模な変更があります。リビューアの仕事は、変更が本当に必要であることを確認し、承認することです。これらの変更は大きくなることがありますが、上記の注意事項(マージやテストなど)は依然として適用されます。

効率的に小さなCLを書く方法

もし小さなCLを書いてからレビュワーに承認されるのを待ってから次のCLを書くとすると、多くの時間を無駄にすることになります。ですので、レビュー待ちの間にブロックされない方法を見つける必要があります。これには、同時に複数のプロジェクトに取り組む、即座に利用可能なレビュワーを見つける、対面でのレビュー、ペアプログラミング、またはCLを分割して即座に作業を続ける方法などが含まれるかもしれません。

CLの分割

相互に依存関係がある可能性のある複数のCLを持つ作業を開始する際には、コーディングに入る前に、高レベルでCLを分割して組織する方法を考えることがしばしば役立ちます。

CLの作成者として、CLを管理・組織するためにも便利ですし、コードリビューアにとっても作業を容易にし、コードレビューを効率化することができます。

以下に、作業を異なるCLに分割するためのいくつかの戦略を示します。

重ねて変更を積み重ねる

自分自身をブロックせずにCLを分割する方法の一つは、小さなCLを書いて、レビューに送り、すぐに最初のCLを基にした別のCLを書き始めることです。ほとんどのバージョン管理システムは、何らかの方法でこれを行うことができます。

ファイルごとの分割

CLを分割する別の方法は、異なるレビュワーが必要ながらもそれ以外は独立した変更を必要とするファイルのグループによって行うことです。

例えば、プロトコルバッファの修正のための1つのCLと、そのプロトを使用するコードの変更のための別のCLを送信します。コードのCLを送信する前にプロトのCLを提出する必要がありますが、両方のCLは同時にレビューできます。これを行う場合、自分が書いた他のCLについて両方のレビュワーに通知することが望ましいかもしれません。これにより、変更の文脈を持っていることができます。

別の例として、コードの変更のための1つのCLと、そのコードを使用する設定や実験のための別のCLを送信します。必要に応じて、設定/実験ファイルはコードの変更よりも早く本番環境にプッシュされることがあるため、必要に応じて簡単に元に戻すこともできます。

水平分割

テックスタックの各層間の変更を分離するのに役立つ共有コードやスタブを作成することを考えてみてください。これにより、開発を迅速化するだけでなく、各層間の抽象化も促進されます。

例えば、クライアント、API、サービス、データモデルの各層を持つ電卓アプリを作成したとします。共有のプロト署名を使用することで、サービスとデータモデルの層を互いに分離することができます。同様に、APIスタブを使用することで、クライアントコードとサービスコードの実装を分割し、それぞれが独立して進めるようにすることができます。同様のアイデアは、より詳細な関数やクラスレベルの抽象化にも適用することができます。

垂直に分割する

階層的で水平なアプローチとは異なり、コードを小さなフルスタックの垂直な機能に分割することもできます。これらの機能のそれぞれは独立した並行実装トラックとなります。これにより、一部のトラックがレビューやフィードバック待ちの間も前進することができます。

水平に分割するでの計算機の例に戻りましょう。今度は、乗算や除算などの新しい演算子をサポートしたいとします。これを、乗算と除算を別々の垂直な機能やサブフィーチャーとして実装することで分割することができます。ただし、共有のボタンスタイリングや共有のバリデーションロジックなど、一部の重複があるかもしれません。

水平および垂直に分割する

さらに進んで、これらのアプローチを組み合わせて、次のような実装計画を作成することができます。各セルは独立したCLです。 モデル(一番下)からクライアントまで進めるようにします。

(訳注)
公式の表が崩れている。
https://google.github.io/eng-practices/review/developer/small-cls.html#splitting-grid
githubの状態に合わせている。
https://github.com/google/eng-practices/blob/master/review/developer/small-cls.md#splitting-horizontally--vertically-splitting-grid

レイヤー機能:乗算機能:除算
クライアント追加ボタン追加ボタン
APIエンドポイントを追加エンドポイントを追加
サービス変換を実装乗算と共有変換ロジックを
: : : 追加する :
モデルプロト定義を追加プロト定義を追加

リファクタリングを分離する

リファクタリングは、通常、機能の変更やバグ修正とは別のCLで行うのが最善です。例えば、クラスの移動や名前の変更は、そのクラスのバグ修正とは別のCLで行うべきです。各CLごとに導入される変更をレビュワーが理解しやすくするためには、分離する方がはるかに簡単です。

ただし、ローカル変数名の修正などの小さなクリーンアップは、機能の変更やバグ修正のCL内に含めることもできます。ただし、現在のCLに含めるとレビューがより困難になると判断される場合は、開発者とレビュワーの判断に委ねられます。

関連するテストコードを同じCLにまとめる

CLには関連するテストコードを含めるべきです。ここでの「小ささ」とは、CLが焦点を絞っていることを指し、行数の単純な関数ではありません。

すべてのGoogleの変更にはテストが必要です。

ロジックを追加または変更するCLには、新しい動作に対応する新しいまたは更新されたテストが必要です。純粋なリファクタリングCL(動作を変更する意図のないもの)もテストでカバーする必要があります。理想的には、これらのテストはすでに存在しているはずですが、存在しない場合は追加する必要があります。

独立したテストの変更は、まず別々のCLに入れることができます。リファクタリングのガイドラインと同様です。これには以下が含まれます:

  • 既存の提出済みコードを新しいテストで検証すること。
    • 重要なロジックがテストでカバーされていることを確認します。
    • 影響を受けるコードの後続のリファクタリングに対する信頼性を高めます。たとえば、テストされていないコードをリファクタリングしたい場合、リファクタリングCLを提出する前にテストCLを提出することで、リファクタリング前後でテストされた動作が変わらないことを検証できます。
  • テストコードのリファクタリング(例:ヘルパー関数の導入)。
  • 大規模なテストフレームワークコードの導入(例:統合テスト)。

ビルドを壊さないでください

もし複数のCLが互いに依存している場合、各CLが提出された後もシステム全体が正常に動作し続けるようにする必要があります。そうしないと、CLを提出するたびに他の開発者全員のビルドが数分間(もしくは後のCLの提出で何か予期せぬ問題が発生した場合はそれ以上の時間)壊れてしまう可能性があります。

十分に小さくすることはできません

CLが大きくならざるを得ない状況に遭遇することがあります。しかし、これは非常に稀です。小さなCLを書くことに慣れた著者は、ほとんどの場合、機能を一連の小さな変更に分解する方法を見つけることができます。

大きなCLを書く前に、リファクタリング専用のCLを前に置くことで、よりクリーンな実装の道を開くことができるかどうかを考えてみてください。チームメンバーと話し合い、機能を小さなCLに実装する方法についての意見を聞いてみてください。

これらのオプションがすべて失敗した場合(非常に稀ですが)、事前にレビュワーの承諾を得て大きなCLをレビューするようにしてください。これにより、彼らは何が来るかについて警告されます。この状況では、長い時間をかけてレビュープロセスを進めることを予想し、バグを導入しないように注意し、テストの作成に特に注意してください。

次: レビューコメントの処理方法