レビューをもらいやすい細かいプルリクの切り分け方
はじめに
コードレビューにおいて、新規機能開発をしているとどうしてもプルリクが大きくなってレビューコストが高くなると思います。 「小さくプルリクを出せ。」と言われても具体的な考え方が整理された知見は少ない気がしています。 小さいプルリクに切り出す考え方について、私自身が先輩エンジニアにレビューしてもらう際に意識していることをこちらにまとめます。
また、PHPerKaigi 2018のLTでの発表内容の詳細になります。 phperkaigi.jp
プルリクに関する前提
本記事は、Gitホスティングサービス(GitHub、BitBucket等)における、「Pull Request」や「Merge Request」をプルリクという言葉で表現しています。
PHPerKaigi 2018の発表資料
目次
- 「大きい」プルリクとは
- 大きいプルリクがなぜ悪いのか
- なぜ大きいプルリクが生まれるのか
- どうやってプルリクを小さくするか
- 小さくしきれないプルリクはどうすればいいか
大きいプルリクとは
大きいプルリクの実例
githubのPull Requestを用いて2つ例をあげます。差分が何千行もあったり変更ファイルが大きいものがざっくり、大きいプルリクだと呼ばれたりします。
Files Changed: 39 Diff: +4,571 -1,676
Files Changed: 3,448 Diff: +3,94 -748,338
上記のプルリクを分解してみると、下の図のように様々な要素が含まれているケースが多いです。
この図をベースに大きいプルリクがどういう特徴があるか見ていきます。
「大きい」プルリクの3つの特徴
- 複数機能が含まれている
- ついでの実装が混ざってる
- 一機能の実装コードが膨大
大きいプルリクがなぜ悪いのか
影響範囲が大きくなる
大きいプルリクには、複数機能・複数観点が含まれます。そのため、プルリクが通ったとしても複数機能の影響範囲を一回のリリースで見ることになります。 また、一機能だけだとしても既存メソッドの修正が複数あればそのメソッド使っている箇所全てが影響範囲として動作保証しないといけません。 結果として、大きければ大きいほどあなたのプルリクをリリースした際に問題が発生する可能性が高まります。
レビュワーの負担がかかる
レビュワーは、あなたが書いたコードをリリースしても良いのか・問題は起きないか・もっと良い書き方ができる箇所はないかという視点でチェックしてくれます。 大きいプルリクの場合は、影響範囲が大きくなるのでそれだけレビュワーの使う労力・時間が肥大化します。
問題がないことを説明する難易度が高くなる
大きいプルリクだと、影響範囲が大きいためそれに対してどのように動作保証をするのかという説明はかなり難しいです。それを、一回のプルリクで説明してレビュワーに理解してもらうのは現実的ではありません。
なぜ大きいプルリクが生まれるのか
結論から、たくさんの要素を一気にやろうとすることが理由です。上の「大きい」プルリクの3つの特徴で見たとおり、複数の要素を一つのプルリクに詰め込んでいくことでサイズが大きくなっています。
どうやってプルリクを小さくするか
小さい要素に切って細かくリリースする 大きいプルリクの特徴が生まれる理由は、一回のリリースで一気にやろうとすることにあります。小さい様子に切って細かくリリースしていきましょう。
小さい要素に切っていくために以下の4つのSTEPを踏んでいきます。
- 1: スコープを定義する
- 2: スコープ外を切り出す
- 3: スコープ内を細分化する
- 4: スコープ内から切り出す
STEP1: スコープを定義する
機能開発・改修するにあたってゴールが無いことは基本的にはないですよね。ここのをしっかり抑えることで、何がゴールに必要で何がついでかを明確に区分することが出来ます。
例）「新規決済手段をリリースする」というゴールの場合、新規決済の注文処理=必要、既存決済部分のリファクタリング=ついで、と区分できますよね。
STEP2: スコープ外を切り出す
「ついでに」やったことはリリースゴールに関係ないので、今回の「プルリク」に必要はありません。
プルリクを分けて事前リリースの対象になります。
ただし、リファクタリングした後のコードに対して、リリースゴールに必要な実装を施す場合は「やっぱりセットじゃないと」という思いもあるかもしれません。
その場合は、リファクタリングのプルリクを別に切り出しておいて、リファクタリングをしたブランチをリリースゴールのブランチにマージして下さい。
一瞬だけプルリクは大きくなりますが、リファクタリングのプルリクが事前にリリースされることによって本流ブランチの差分からは外れるので結果としてプルリクは小さくなります。
STEP3: スコープ内を細分化する
スコープ内を細分化するには、スコープ内にも先に出しておける機能がないかを見極めることが大事です。 切り出せる対象は、既存の振る舞いを変えないものです。 例えば、新規モジュールを作って利用するのであれば最後のプルリクでやらなくても事前にリリースできますよね。
以下、切り出せる単位の例を記します。
例1: 新規クラスの場合
新機能を実装するにあたり、新規クラス・メソッドの実装、既存クラス・メソッドの修正のどちらかになるかと思います。 新規クラス・メソッドの実装であれば、呼び出しが行われないので事前にリリースしても大丈夫なケースが多いですよね。
例2: 既存クラス・メソッドの修正
既存クラス・メソッドの修正の場合は、 - 既存処理の振る舞い（特にI/F）を変える場合：呼び出し元の修正とともにセット - 既存処理の振る舞いは変えない：単体でリリース可能
STEP4: スコープ内から切り出す
依存関係の整理にて、切り分けた単位がそのままプルリクになります。事前リリース可能対象をどんどんdevelopmentにマージしていきましょう。 その際に、ただし機能ごとではなくクラスごとのリリースになるので、テストコードがセットになっていることが理想です。 また、レビュー済みの事前リリースが住んでいるのであれば、参照元のメソッドがレビュー済みな状態で次のコードのレビューを開始できます。
小さくしきれないプルリクはどうすればいいか
上記の方法でやっても、どうしても小さく出来ないことはあります。例えば、「新規決済の実装」という場合は、注文・キャンセル・配送・与信延長等、決済フローが1セットで1機能です。さすがに注文だけリリースするけど配送は後でということは出来ませんね。 その場合は、コードの差分を「どん！」と渡してもレビュワーは正確にあなたのコードを見きれません。レビュワーに伝える努力が必要です。
トピックブランチの活用して要素ごとにレビューを出す
トピックブランチは、案件専用ブランチです。実装している機能に対してトピックブランチを用意します。
このトピックブランチに対して、要素ごとに派生ブランチを切ってプルリクを出すことが有効です。
例えば、「新規決済の実装」であれば、注文部分を実装したプルリク・キャンセル部分を実装したプルリク・配送部分を実装したプルリク・与信延長部分を実装したプルリクをトピックブランチに出してそれぞれレビューしてもらいます。
そうすることで、レビュワーは要素ごとにレビューをすることができるため、細かい点までレビューする事が可能になります。
コミット単位で要素を分ける方法もありますが、レビューコメントがもらいにくいため、機能実装のレビュー記録・進捗が見えにくい点があります。 やっておいたほうがいいことは、派生ブランチはマージ後に削除しておいたほうがブランチがきれいに保たれるので良いです。
自分の書いたコード以外で発生する差分はそれだけで一コミットにする
不要なライブラリの削除やライブラリのgit管理下に置く場合は、それだけでかなりの差分になります。この差分に対して自分が書いたコードを混ぜた暁には埋もれます。 あなたの書いたコードを探すこと自体でレビュワーはゲンナリです。自分の書いたコード以外で発生する差分は明確にコミットを分けるようにすることが有用です。
例、git管理下のライブラリを削除してcomposer installする
また、余計な変更が混ざっていないことを可視化するためにプルリクエストの説明内に下記のようにgit statusの出力をgrepした結果を載せることも有効です。
$ rm -rf app/Vendor/aws $ git status | grep -v app/Vendor/aws On branch development Your branch is up-to-date with 'origin/development'. Changes not staged for commit: (use "git add/rm <file>..." to update what will be committed) (use "git checkout -- <file>..." to discard changes in working directory) no changes added to commit (use "git add" and/or "git commit -a")
