2017.02.07

より良いコードレビューを依頼するために学んだこと

亀田瑞貴

2016年度新卒入社の亀田です。現在、横断アーキテクトGという部署にて、共通ライブラリの開発に携わっています。

自分は入社するまでチーム開発の経験が非常に浅く、入社後にコードレビュー依頼の仕方について指導を受けることが少なくありませんでした。今回は、この一年で学んだ、良いコードレビューを行うためにレビュイー側が気をつけるべきことについてお話します。当たり前の事も多々含まれているかと思いますが、どうかお付き合いください。

リクルートライフスタイルではGitHub Enterpriseを使用した開発を行っており、今回の話もGitHubを想定しています。

1. Pull Request 作成の前に

Pull Request (PR) 作成の前の段階では、以下3つの事を気をつけています。

レビュアーと実装方針を予めすり合わせる

議論の余地がありそうな部分に関しては、レビュアーと実装方針を予めすり合わせましょう。実装方針にそもそもの誤りがあった場合、実装し直しとなり無駄が大きくなってしまいます。これまでの開発から、この観点は特に重要だと考えており、マージまでのスピードやコード品質が大きく違う実感があります。

PRの粒度は可能な限り小さく

PRの粒度は可能な限り小さくしましょう。PRの粒度が大きくなってしまうと、1つのPRに複数の目的が混ざってしまったり、コード差分が大きくなってしまったりします。そうすると、どこを中心にレビューして良いかが分かりづらく、レビュアーの負担が大きくなってしまいます。

WIPを活用する

実装上の難易度から議論の余地が大きいと感じた場合、Work In Progress (WIP) PRとして早めにPRを出しましょう。 WIP PRとは、PRタイトル頭に “WIP” の文字列を含めることで、作業途中であることを明示しているPRのことを指します。ある程度の実装の段階でWIP PRとして何度かレビューを受けることで、都度軌道修正を行うことができ、これによって大きな手戻りを防ぐことができます。

WIP PRを出す場合、現在の進捗状況(何が終わっており、何が終わっていないか)や、特に見て欲しいところをPRのコメントに書くようにしましょう。作りかけの部分が混在する(ノイズが多い)WIPの性質上、レビュアーはどこを集中して見てよいか分かりづらく、これら情報が欠けるとレビュアーの負担が増えてしまいます。

2. レビュー依頼の前の確認

以下の項目を全て満たす状態でレビューを依頼するようにしています。コードレビュー依頼以前に、これらの項目を満たしていないコードが本番に上がることは好ましくありません。レビュアーが本質に集中してレビューが出来るよう、これらの項目は必ず満たすようにします。

コードによって本当に目的を達成しているか

レビュー対象の修正にて本当に目的を達成しているか、再度確認を行いましょう。稀に、何らかの過程で問題をすり替えてしまっている場合があります。

コード規約に準拠しているか

共通のコード規約が存在する場合、コード規約に準じているか確認を行いましょう。コード規約から外れている場合、非本質的な問題が目立ち、本質的な問題の発見が難しくなる可能性があります。当然ながら、レビュアーの負担も増えてしまいます。これに関してチーム共通のLintにて自動的に担保できればベターです。

また、同様の理由でコードフォーマットを揃えるようにしましょう。本質的な問題に集中するため、チームで共通のフォーマッタを利用するなどし、不要な差分が出ぬ仕組みを用意しましょう。

ユニットテストを行っているか

特に実装した範囲に関して、カバレッジの計測を行いましょう。ほぼ網羅しているつもりでも、計測してみると想定外の部分が漏れている場合があります。また、改修したクラスやそれを参照するクラスのみならず、それらが含まれるプロジェクト全体のテストを通すようにしましょう。

全てのコード/ファイルが上がっているか・不要なコード/ファイルが上がっていないか

レビューに必要な全てのコード/ファイルがGitに上がっているか今一度確認を行いましょう。また、不要なファイルがGitに上がっていないか確認をしましょう。不要なファイルは .gitignore に記述するなどし、不要なファイルを上げてしまうことを自動的に防げるようにするとベターです。

print文や一時的なTODOコメントなど、作業用の一時的記述が差分に含まれていないか確認をしましょう。Gitのpre-commit hookをtemplateに追加するなどし、これらを自動的に担保できる仕組みがあれば、より楽ができます。自分の場合、pre-commit hookとして、以下のコード片を追加し、コミット対象のファイルにprint文が残っていないかチェックするようにしています。

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
if git rev-parse --verify HEAD >/dev/null 2>&1
then
  against=HEAD
else
  # 注1: 定数4b825(以下略)は変更しないでください
  # Initial commit: diff against an empty tree object
  against=4b825dc642cb6eb9a060e54bf8d69288fbee4904
fi
# 注2: 禁止したい単語をbanned_wordsに登録してください
banned_words=("System.out.println")
contains_bw=0
# 注2: 必要に応じて以下の行の"grep \.java$"を任意の言語の拡張子に置換してください
for FILE in `git diff-index --cached --name-status $against -- | grep '\.java$' | cut -c3-` ; do
  for bw in "${banned_words[@]}" ; do
    if grep $bw $FILE ; then
      echo $FILE ' contains ' $bw
      contains_bw=1
    fi
  done
done
exit $contains_bw

(注1: againstの定数"4b825dc642cb6eb9a060e54bf8d69288fbee4904"はempty treeを表す特殊なハッシュ値であるため、環境毎に変更を行う必要はありません。参考URL )

(注2: 上記スクリプト例は、Javaを対象としており、".java"の拡張子を持つファイルに対して"System.out.println"の検出を行うようにしています。他の言語を対象とする場合、禁止する単語を適切に設定し、"grep ‘.java$’“の拡張子部分を、その言語の拡張子に変更してください。)

差分の目視確認

差分の目視確認を行いましょう。ミスに気づくことがあります。

3. レビュー依頼

レビュー依頼をする際には、以下の2点に関して気をつけています。特に「いつまでにレビューして欲しいかを明示する」点は非常に大切だと考えています。入社したての頃はこの習慣がなかったため、レビュアーの負担を大きくしてしまっていました。

いつまでにレビューして欲しいかを明示する

「いつまでに」の観点を抜かしたレビュー依頼を行うと、お互いに期日を楽観的に考えてしまい、遅延してしまう可能性があります。また、レビュアーが多忙な場合、スケジュールも立てづらく負担となる可能性があります。期日を明示するようにしましょう。

レビューが急ぎでない場合、期日と併せてそれを明記するとベターです。

目的(何を実現するPRか)、背景(なぜやるに至ったか)、あれば、特に見てほしい点を明確に伝える

レビュー依頼が「レビューをしてください」だけの場合、何を中心に見ればよいかわかりにくく、レビュアーの負担が増えます。目的・背景・特に見て欲しい点をPRのコメントに明記するようにしましょう。これによって、レビュー漏れの可能性をより小さくすることができます。

最低限、以下の項目は埋めるようにしましょう。

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
## 概要
この変更によって、何がどう解決されるか(目的)を記述します。
## 背景
なぜこの変更を行うのか(背景)を記述します。
また、関連チケットやissueなど、PRの背景に関する他のURLがあればそれを貼ります。
## 現在の進捗　(WIPの場合のみ記述)
何がどこまで終わっているか、これから何をしようとしているのかを、レビュアーに示します。
タスク単位で箇条書きし、Markdownのタスクリスト機能を使うと便利です。
例. タスクリスト機能を用いたタスクの管理
- [x] 完了タスク
- [ ] 未完了タスク
## 特に見て頂きたい点  (WIPの場合必ず記述)
論点があるなどして、特に見て欲しい部分がある場合記入します。
議論したい(見てほしい)実装がGitHub上に存在する場合、そのURLも併記しましょう。
## レビュアー
必須レビュアーのレビュー漏れを防ぐため、必須レビュアーの方々を明示するようにしましょう。
レビュアーがレビュー完了したことを示すため、Markdownのタスクリスト機能を使うと便利です。
例. タスクリスト機能を用いたレビュアーの列挙
- [x] @hogehoge さん    (チェック状態はレビューの完了を表します)
- [ ] @fugafuga さん    (非チェック状態はレビューの未完了を表します)

4. レビュー中〜再レビュー依頼

レビュー中〜再レビュー依頼までの間は、以下の4点に関して気をつけています。特に、「レビューの指摘事項は、修正方針のすり合わせをできるだけすぐに行う」点は非常に重要だと考えており、マージまでの時間短縮に効果的だったと実感しています。

レビューの指摘事項は、修正方針のすり合わせをできるだけすぐに行う

レビュアーが指摘事項を覚えているうちに、なるべく早く修正方針のすり合わせを行いましょう。これを行わない場合、的外れな修正を行ってしまう可能性があり、再修正の無駄が生まれてしまいます。

指摘コメントに必ず返信をする

レビュアーからすると、指摘コメントを見たかどうか分からないため、レビュー対応漏れの可能性に繋がります。たとえ論点が無く修正する場合にも、対応した旨の返信をつけましょう。

口頭で貰ったレビューコメントに関しては、忘れないようGitHubにコメント化

口頭で貰ったレビューコメントは忘れてしまう恐れがあります。指摘事項をPRのコメントに残すようにしましょう。

再レビューの依頼に差分(/compare機能)を使う

再レビュー依頼は、適切ならば /compare 機能を使うなど、差分で依頼するようにしましょう。レビュアーの負担が減ります。

5. レビュー完了

マージ完了をしたら以下の2点を行っています。特に、2点目の「PRにて指摘されたことのリストを残す」に関して、今後同傾向のミスを減らすための振り返りがしやすく、効果的だったと考えています。

マージしたことを伝える

レビューのお礼とともに、マージしたことを伝えましょう。

PRにて指摘されたことのリストを残す

残したリストをチェックすることより、次回以降に同じような指摘を事前に防ぐことができます。また、リストの蓄積から自分の指摘されやすい傾向が分かり、改善に繋げやすくなります。

(番外編)日頃から

レビュアーと気軽に相談・議論ができるよう、日頃から聞きやすい関係を作っておくとよかったです。個人的には、一緒にランチに行くと仲良くなれることが多かったです。品質の高い仕事をするために、これが一番大切だったかもしれません。

おわりに

入社以降、レビューの受け方に関して多くの指導を受け、現在は以上のことを心がけてレビューを依頼するようにしています。レビュイーで担保できる点はレビュイーで担保することで、多忙なレビュアーからより早く本質的なレビューを受けられるようになったと実感しています。レビュアー・レビュイーとも、より気持ちよく開発ができるよう、今後も試行錯誤を続けてゆきます。

タグから見つける