2020.10.10

PRチェックリスト

自戒も込めて、過去の自分に対して書いてみる。

エンジニアとしてはたらき始めてから、PR でいろいろなことを指摘されてきた。ここではその指摘をまとめて、いつでも見返せるようにしてみる。このページを逐一確認すれば、徐々に指摘されるポイントは減っていくか、またはより高度なものになっていくはず。後輩の PR に指摘するときにも役立ちそう。

開発スタックは Go と React

PR をお願いするときの大前提

コードレビュー依頼とは、いわば責任の共有依頼です。<中略> コードレビューを通したレビュアーには、品質の保証をしたという責任が伴います。通したコードが不利益を生んだ場合には、レビュアーも責任を負わなければなりません。

コードレビューを依頼するときの心構え - Qiita

だからこそ、レビューしやすく、レビュワーに負担をかけない PR を作ることが求められる。

PR チェックリスト

あたりまえのことだけれど、PR のレビューをお願いする前にかならず自分がチェックすること。レビュワーはミス発見器ではないので、指摘されるまでもないミスはレビューをお願いする前に解決しておく。

PR のタイトルは適切か

PR の内容を適切に表現したものになっているか
フォーマットにしたがっているか（チケット番号を Prefix にするなど）

PR のサイズは小さいか

大量のコードを一度にみせるのはレビュワーに負担をかける。大量のコードは問題を隠してしまう。小さく作り、小さく見せる。小さく区切ることによってレビューの精度が上がる。

競合はないか

この指摘は不毛。かならず解決してからレビューをお願いする。複数人で開発する場合、レビューしてもらっている間に master が更新されて競合が発生したりする。

コミットログは適切か

作業中に一時的にコミットしたものがそのままになっていないか
不適切なコミットメッセージがないか
まとめるべきコミットはないか

デバッグコードが残っていないか

検索して調べておく。

コード内に消し忘れたコメントはないか

残すべきでない TODO や FIXME がないかどうか、検索してみる。以前、自分用に雑に書いたコメントが master にマージされてしまい、恥ずかしい思いをしたから、これは厳重にチェック。

変数名は適切か

typo はないか
命名規則にしたがっているか
不適切な名前を使っていないか

メソッド名は適切か

命名規則に反していないか。たとえば、同じ取得処理でも返り値が複数のときは Find、単数のときは Get を使うし、同じ生成処理でも Make、Build、Generate を使いわける。
よりわかりやすいメソッド名はないか。メソッド名にできるだけ多くの情報をつめこんで、メソッドの役割や挙動が明確に伝わる名前をつける。 CreateOrUpdate よりも Upsert のほうが簡潔でみやすい。

メソッド引数は適切か

不要な引数が残っていないか。不要な変数が残っていたらコンパイルエラーになるが、不要な引数が残っていてもコンパイルエラーにはならないので気づきにくい
命名は適切か。ほかの箇所と一貫性があるか
順番は適切か。context.Context はいちばん最初で、次に time.Time、その後は重要度順

変数の定義場所は適切か

できるだけ使う直前に定義する。ただし、エラーをともなう場合はその限りではない（無駄な処理を走らせなくないから）。

ループの外で定義してもいいものを、ループの中で定義していないか。

処理の順番は適切か

バリデーションは処理の最初にいれて、無駄な処理を走らせない
メソッド内であれば、引数の順番に対応させるとよい
クエリの実行順には気を遣う。Find と Count があったら、Count を先に実行する

処理はわかりやすいか

行数の少なさや簡潔さよりも、わかりやすさのほうが大切なことがある。リーダブルコードであるかどうかを判断基準にする。

- address := v.address.ToString()
- if address == "" && v.Freeddress != nil {
-   address = *v.FreeAddress
- }

+ var address string
+ if s := v.address.ToString(); s != "" {
+   address = s
+ } else if t := v.FreeAddress; t != nil {
+   address = *t
+ }

エラーハンドリング漏れはないか

if err != nil { return err } で返す値は正しいか。たまに nil を返していたりする
ハンドリングできていないパターンはないかどうか。漏れていないか。

エラーの構造体は適切か

独自のエラーの構造体を使っている場合、適切な構造体を使っているかどうかチェックする

ドメインルールがドメイン層の外に出ていないか

気をつけないと漏れがち。

logic の使い方は適切か

logic には値の処理だけを書く。 ToPb() や UpdateAddress() など
logic にふくめる処理はできるだけ少なくする。たとえば、slice を map にする処理は logic の外で行い、logic には map を渡すなど

logic と domain service の使い分けは適切か

logic に書くべきでないドメインルールを domain service で共通化するくらいの認識。

権限の異なる複数のユーザで挙動を確認したか

システムによっては「管理者」と「一般ユーザ」のように権限が異なる複数のユーザが存在したりする。権限によって適切にリソースが制限されているかどうか、または、されていないかどうか、かならずあらゆる権限のユーザで挙動を確認すること。

ディスプレイの解像度はデフォルトか

UI はディスプレイの解像度によって見え方が変わるので、かならず一般的に使われているディスプレイの解像度で開発すること。

ブラウザのズームレベルは 100%か

前項に関連して、ズームレベルも一般的な 100% で開発すること。以前、個人的な好みで 90% にして開発していたら、それが原因でチームメンバーと同じ挙動にならなかったことがある。本番環境でも異なる挙動になるので、原因がわかるまで時間がかかってしまった。

実際の挙動をチェックしたか

ちょっとした修正でもかならずコンパイルしてみること。 Modules の replace まわりで動かなくて、パッケージのバージョンを上げていないことに気づいたりできる
ちょっとした修正でもかならず動かしてみること。想定通りに動くだろうと頭だけで判断すると痛い目に合う。

インデックスの貼り方は適切か

インデックスは適当に貼っても意味がない。むしろ速度が落ちる。テーブルにカラムを追加したときは、そのカラムにインデックスを貼る必要があるかどうか考える。

Seed でデータ不整合が発生しないか

Seed をいじったときは、かならず動かしてみること。 UI でデータを制御するのと違って、Seed は不正なデータを入力できるので、不正なデータが入らないように気をつける必要がある。