自己紹介
株式会社 Signifi でエンジニアとして働いています。
昔はバックエンド専門でしたが、最近はモバイルアプリ、Webフロント、DevOpsなども担当しています。
背景
コードレビューをする時に、プルリクエストを見て「何をレビューしたらいいのか分からない！」ってことはありませんか？
ここでは、レビューワーがどんな情報を期待しているのか？実装者はどんな情報を記載すればいいのか？
その辺りで自分が普段気をつけている内容を記載しようと思っています。
レビューとは？
まずは、よくあるそもそも「レビュー」とはなんぞやというところから記載します。
ちょっと google 検索をすると下記の内容が出てきます。
情報システムやソフトウェアの開発で、作成された仕様書やコードなどの成果物を開発者とは別の人が詳細に調べ、仕様や要求を満たす内容になっているか、誤りや不具合が無いか、資源の浪費や不必要な冗長さ、極端な低性能などの問題が無いかなどについて開発者にフィードバックする工程をレビューという。
とはいえ、この内容をレビューワーが愚直にやるのは負担が大きいため、厳しいかと思います。
実装者側で担保して欲しいこと
そこで自分は下記をなるべく明言するようにしています。
動作確認
コードが意図した通りに動作するかどうかは、単体テストで実装者が確認すべき事項だと思っています。
もちろん、間違いを見つけたら指摘はしますが、レビューで弾く想定だとレビューワーはかなり大変です。
最低限の動作保証は、実装者側の責任でお願いしています。
実装背景を明記
「なんでこんな回りくどいことしているんだろう？」...そう思った事はありませんか？
「なんでこんなことしているんですか？」...って聞いたら驚愕の事実を聞いたことはありませんか？
「最初からプルリクエスト文面に書いてよ！」...って思った経験はありませんか？
私はたくさん経験があります。
そこで、「実装をするときに気をつけなければならなかったこと」をきちんと書くようにしましょう。
例えば、下記のような事項です。
登録顧客の一覧を検索する API を作ったとして、下記のように書かれていればどこに何の修正を入れたか想像できませんか？
会員登録を介して登録された顧客一覧を名前から検索する機能を作りました。
会員数は最終的に500万人を見込んでおり、SQL の Like 文ではパフォーマンス的に対処できない想定のため全文検索を使って検索するようにしています。
このため、会員登録時でも検索をするための仕込みの処理を追加しています。
また、API 側であるデータが削除されるので、それらに関連するプルリクエストでは下記のように書いたこともあります。
○○ API の ××属性が削除されるため、該当データを参照するすべての内容を削除しています。
ただし、API 側の該当データ削除は、フロント修正後に行われるため API は現状態でも動作するように記載しています。
(※ 削除予定の必須パラメータにはダミー値を入れたりしている)
この部分に関しては、TODO コメントとして修正すべき内容を記載して、API 側で修正されたらフロント側も修正する想定です。
このように書いておくだけで、「何でこんなことをしているんだ？」という疑問の大半は解消されるはずです。
コードの細かい説明は不要
「実装に関して説明を書いてください。」と言うと、一行一行実装内容を説明する人がいます。
これは、私は不要だと考えています。
なぜなら、プルリクエストに細かい挙動の説明をしないと、意図が理解できないソースコードはそれだけで危険なものです。
また、プルリクエストの説明はコードレビューでしか見る事はできません。
あとでその箇所を修正するとなったとき、理解できなかったら大変な事です。
どうしても分かり辛いことは、一行コメント・ドキュメンテーションコメントで対応するようにしましょう。
ただし、これはあくまで次善策であり、本来はソースコードだけで意図が伝わるようにするのが理想です。
不安なこと・課題点は明記する
時間は有限です。どうしても完璧に実装する事は難しいでしょう。
それはそれで許されるケースもあります。不安な事は不安として記載しましょう。
簡単に解決できるなら、レビューワーが指摘をくれるでしょう。
課題を解決したいけど、それが難しいケースもしくは優先度的に後回しでも問題なければ、
別タスクとして定義して TODO コメントをつけるなり対応できるはずです。
その選択肢で悩んでいる！と言う情報があると、レビューワーも読む時に理解してくれるはずです。
まとめ
今回はコードレビューに関して、実装者側で意識した方がいいことを記載しました。
コードレビューはレビューワーの負担が大きく、最終的に頓挫してしまうケースも多いと思います。
しかし、コードレビューの本来の目的はソースコードを開発者間で共有意識を持ってメンテナンスしていくことです。
これを目指すのであれば、レビューワーだけでなく実装者側もできるだけレビューワーに優しいプルリクエストを書くことをお勧めします。