レビューしやすいプルリクエスト

普段レビューをしていて、レビューしやすいプルリクエストに対して個人的に感じている特徴をまとめてみました。

#Git

#ポエム

稲葉純

2020.07.15

この記事は公開されてから1年以上経過しています。情報が古い可能性がありますので、ご注意ください。

普段レビューをしていて、レビューしやすいプルリクエストに対して個人的に感じている特徴をまとめてみました。割と大きめなソースコードに対するレビューの話が主となります。

ざっくりまとめ

本記事では以下のようなトピックについて記載しています。

差分の目的が1つ
- レビューをしながら「私はいま何のレビューをしているのか」のコンテキストスイッチが発生しないので嬉しい
何を達成したいのかがわかる
- レビューの多くは「やりたいこと」と「実現方法」のすり合わせなので、前者の精度を上げたい
分割されすぎていない
- 他のコードとの関連性や構造についてのレビューがしやすい
レビューの強弱をつけるための情報がついている
- 機械的な変換の差分だったりした場合、それが事前にわかると嬉しい
検証結果が書いてある
- コードだけで担保できない部分について「ほんとにこれ動くの？」とかがわかると嬉しい
ファイル間の関連がわかる
- 変更したファイル数が多い場合、何が含まれていて、何がコアなのか、といった情報がわかるとレビューに取り掛かりやすい
この後の展開がわかる
- マージ後のタスク（検証だったりドキュメントの執筆だったり）がわかると、それを前提にレビューできて嬉しい

差分の目的が1つ

いわゆる凝縮している、というやつでしょうか。例えば1つのPRに「Aの機能追加」と「Bの不具合修正」を混ぜないようにする、といった話です。

PRのコメントでそのような修正が含まれていること、くらいまではレビュアーに伝えることができますが、実際にコードを見ている時「これはAの機能追加の差分なのか、Bの不具合修正の差分なのか」を判断しながらコードを見るのは結構骨が折れます。 2つくらいならともかく、更に一括置換のリファクタリングで差分が膨らんで、みたいなものが入ってくるとコードを見ていてなかなか混乱します。

得に、PRの中のコードに対して依存関係がある場合、どれがどれに依存しているのか、を意識しながらレビューを行うことになりますが、「XはYに依存していて（Aのコンテキスト）、さらにYはZに依存している（Bのコンテキスト）」といったように、1つの差分を見ながら「自分は一体何をレビューしているのか」を細かく切り替えながらのレビューはなかなか大変です。

何を達成したいのかがわかる

PRには通常何か達成したいゴール（機能追加であったりバグ修正であったり）があります。レビューはそれを実現するためのコードに対して実現する手段の可否やそもそも実現できるのか？を確認する作業です。その時、実現方法以前に「何を実現したいか？」が不明瞭だとコードのレビューもそれに合わせて曖昧になったり、レビュアーとレビュイーの達成したいゴールの認識がずれたりします。

この認識がずれていると、PRの内容がいい感じかどうかのレビューは絶対に精度がでません。

何をしたいのか？の表現方法はPRのコメントであったり、差分に含まれる仕様であったり、もしくはどこか他のところにある仕様かもしれませんが、PRから「何を実現したいか？」がわかると、レビュアーとしては非常に助かります。

分割されすぎていない

これは人によると思うのですが、個人的には分割されすぎたPRはレビューがしづらい、と思っています。レビューがしづらいというのは「レビューにかかる時間が増える」という意味ではなく「レビューの精度が落ちる」気がする、ということです。あまりに細かく分割されたコードは、全体的な構造の見通しが悪いと感じることがあります。

個々のコードは問題がなくても全体を通して見た時に構造やクラスの分割、抽象化などがいまいちで、それに気づかなかったり後で直したくなったり、といったことがあります。

例えば、このようなコードを追加する差分があったとします。

public class UserStore {
    public void deleteUser(String userId) {
        // ユーザーを物理削除する
    }
}

このコードを見た時に、レビュアーは「ユーザーを削除するコードが追加されたこと」「ユーザーを削除するコードとして、deleteUser が正しいかどうか」あたりは判断できます。しかし、「そもそもこれ、いるの？」とか「物理削除でいいの？」といった観点でのレビューは、このコードだけだと難しいです。どういったコンテキストでこのメソッドが追加されたのか、判断ができないからです。

コードを利用する側の差分があった場合どうでしょうか。

public class UserController {

    UserStore userStore;

    /**
     * ユーザーが退会ボタンを押したときのハンドラー
     */
    public void resign(String userId) {
        userStore.deleteUser(userId);
    }
}

ユーザーが自主的に退会ボタンを押した時に deleteUser が呼ばれる、という情報が追加されます。ここでレビュアーは、「退会って単純に物理削除でいいのか・・・？」など、もう少し広い観点でのレビューをしやすくなります。 resign の実装時にも気づくことはできるのですが、根本的なところから考え直したい場合、 deleteUser のレビューにかける労力も減らすことができるかと思います。

また、上記は割と安直な例ですが、アスペクト処理であったりいろいろ抽象化していたり、使い方を想像しにくいコードの場合、ある程度1つの差分に修正がまとまっていてほしいと感じるケースが多いです。

具体的にどれくらいまでのボリュームなら分割しないほうがよいのか、については完全な私の主観ですが、差分が2000行くらいまでなら下手に分割するよりもまとまってたほうがよいかな・・・という感覚です。ただしその場合、この後に出てくるような情報がPRに付加されていると非常に助かります。

ただし当然PRは小さいに越したことはなくて、差分の目的が複数にまたがっていてPRが肥大化している場合は目的ごとに分割したほうがレビューはしやすいと思います。

レビューの強弱をつけるための情報がついている

主に巨大なPRについての話です。

レビューを始める前の段階で「どこを重点的にレビューしてほしいか」「そんなに真面目にレビューしなくてもよい場所はどこか」などがレビュー前にわかるようになっていると非常に助かります。例えばリファクタリングのレビューであれば、それが人力で行われたものなのか、一括置換で行われたものなのかでレビューの力加減を変えられるのではないでしょうか。

組織によりますが、どうしても巨大なコードのレビューは部分的に甘くなってしまうことがあります。このあたりの情報がついているとレビューの精度をあげるためにも、レビューの工数を減らすためにもう有用だと思います。

検証結果が書いてある

そのコードによって何が起こるのか、問題が起きないのか、など全てがコードによる自動テストで担保されていればそれは素晴らしいですが、そうでないケースもあります。

その時、実際に試して動作がどうだったのか？問題ないのか？、条件付きで問題ないのか？、問題があるのか？（この場合のPRのシチュエーションはよくわからないですが）がわかると、レビュー時に助かります。レビューの強弱の話に似ていますが、コードを見る時にこのコードはこういった検証をしてこのような結果がでた上でのコードであるという姿勢でレビューをするのと、このコードはちゃんと動くのかどうかわからないという姿勢でのレビューは、見る観点がだいぶ変わってくるので、レビューにかける時間、精度ともに有効な情報だと思います。

逆に、「こういう検証は何か事情があってやっていない」という話も書いてあると、それはそれで「その観点でのレビューは重くする必要がある」という情報として有用だと思います。

ファイル間の関連が書いてある

こちらも主に巨大なPRについての話です。

そもそも大した差分でなければあまり気にしなくても良いと思いますが、1つの PR に複数のファイルの差分が含まれる場合、レビューをするにあたってファイル間の関連を理解する必要があります。

純粋な機能の追加であれば、どのコードがどのコードにどう関係しているのか、どのテストがどのコードに対応しているのか、などです。例えばコアになる実装修正1つと大量のテスト、のようなPRに対して「コアになる実装」を差分の中から探さないでよい状態になっていると非常に助かります。

実装と単体テストを交互に見ていくようなレビューをしがちなのですが、対応する単体テストがファイル名だけで判断できない場合、それを探すこと自体が面倒だったりするので、この場合何か対応がわかるようになっていると非常に助かります。

もしくは、ドキュメントも含まれる場合コードとドキュメントの対応など理解する必要があるかもしれません。

この関連を理解する、という工程は差分に含まれるファイル数が増えれば増えるほど時間がかかるようになります。特に差分が大きい場合、「何が含まれるPRで、どれが何に依存して、どのドキュメントがどれに対応して」、といった情報のサマリがわかるとレビューをはじめるときの方針が立てやすいです。

この後の展開がわかる

PRによりますが、一連の関連するタスクがPRのマージで完了、ではないケースがあります。例えば納期の問題でなるべくすぐマージしたいが、後続で「テストを自動化したい」「ドキュメントを書きたい」「リファクタリングしたい」といった場合です。

この「したい」が実現されるかどうかはともかく、レビュアーとしてはミスで何かが足りないのか、レビュイーが意図的にそのようにしているのか、今後そこが修正される予定があるのか、がわかると、レビューの姿勢に影響があります。

NGのレビューをするのはコメントのやりとりなども含めてそれなりにコストなので、事前の情報である程度作業を減らせると嬉しいです。

また、コードそのもの以外でも処理時間にセンシティブな実装に対して実環境での検証予定があるかどうか、などの情報がわかると、少なくとも本番環境で処理時間に関する事故が起こる可能性、についての考慮を下げたレビューが行える、などのメリットがあると思います。（いずれにせよそういった観点でのレビューは必要だとは思いますが）