この記事は Nifty Advent Calendar 2017 16日目の記事です。昨日は @nokiyu_oO さんの「非エンジニアがフロントエンドの研修を受けてガチャを作れるようになるまで」でした。
まえがき
なぜ作ったか
この「コードレビューのガイドライン」を作ったきっかけは、とあるスプリントのふりかえりでした。とあるスプリントで、本来ならコードレビューの時点で潰しておきたかったレベルのバグを、うっかり埋め込んでしまったのです。これに対する改善アクションとして、チームでコードレビューをするときのガイドラインを明文化することになりました。明文化によってコードレビュー時に意識すべきことが明確になり、レビュー精度の向上が期待できます。
タイムリーだったのでこれを Qiita で共有してみようと思い、この記事を書きました。
どうやって作ったか
ふりかえりのアクティビティの1つである「555 (トリプルニッケル) 」を参考にしました。
- 紙とペンをチームメンバーに配る
- 「コードレビュー時に心がけていること」を書き出す (5分)
- 隣の人に紙を回す
- 隣の人が書いた内容を読んで、意見・感想・思いついたアイデアなどを自由に追記する (5分)
- 最初に書いた紙が自分に戻ってくるまで 3 と 4 を繰り返す
- 全員の紙を並べて眺めて、意見を集約させていく
記事冒頭の画像が、このときに実際に書き出された意見達です。
前書きが長くなりましたが、以下が最終的にまとめられたガイドラインです。
コードレビューガイドライン
読み手が気にすべきこと
文字表現
変数名、関数名はわかりやすいか
- 英文として意味の通じる表現になっているか
- × :
s1 - ○ :
user_id
- × :
- いわゆる「カラフルな」表現になっているか
- × :
data - ○ :
num_of_unique_users
- × :
コメントは適切に書かれているか
- 正しい文法の日本語で書かれているか
- ×:
ここまで流れて認証エリア以外は、API接続して終了します - ○:
ここまで処理が到達して、かつアクセス先が認証エリア以外だったら、○○APIに接続して終了
- ×:
- 初見で処理内容がわからなかった箇所にコメントがちゃんとあるか
- もしなければ、「ここにコメントがあってもいいのでは？」と提案する
マジックナンバーが使われていない
- いわずもがな
設計・実装
DRY かどうか
- 同じ処理が何度も出てきていないか
- もし重複が激しければ、メソッドの切り出しを提案する
- 標準機能でできることを自力で実装してないか
- それを指摘するためには、レビュワーがその言語にそれなりに詳しい必要がある
最適化を求めるあまり読みやすさが損なわれていないか
- DRY を追求しすぎてかえって複雑なコードになってないか
- 汎用性を求めて YAGNI の原則に背いていないか
ネスト地獄になってないか
- 深いネストは読みづらい
- if 文のネスト
- 1つにまとめられる if 文はまとめてもらう
- ガード節 を導入して、エラー時にすぐ return するようにしてもらう
- ループのネスト
- 多少は仕方ない
定数は正しいか
- 例
- URL
- ファイル名
- 設定値
- 環境ごとに設定すべき値が変わる場合、それが考慮されているか
typo はないか
- 動作に影響のあるレベルの typo は動作確認で検出可能なので、血眼になって探す必要はない
- 何も指摘事項が見つからなくてもせめて typo くらい指摘してやろう、くらいのテンションが重要（精神論）
動作確認
- レビュワー側でも動作確認 or テスト実行する
書き手が気にすべきこと
- プルリクの説明に下記のことを書くとよい
- 動作確認手順
- 自分のコードの不安な箇所
人間が気にすべきでないこと (= ツールでやるべきこと)
- コーディング規約のチェック
- if 文のフォーマット
- インデント
その他
- よいコードを書くためにも、よい指摘をするためにも「リーダブルコード」は必読
あとがき
実際に書き出してみると、一般的に言われていること以上のものは特に出てきませんでした。それでも先述のとおり、明文化すること自体に価値があったのではないかと考えています。特に新人エンジニアのレビュー精度の向上が期待できます。
唯一、我々のチームならではの思いが表れたのが「最適化を求めるあまり読みやすさが損なわれていないか」という項目でしょう。今、我々のチームは10年来の技術的負債に日々苦しめられています。例えば、「データベースからレコードを取得してファイルに吐くだけのバッチ」が平気でC言語で実装されています。そんな歪んだシステムを相手にする中で、シンプルさへの憧れが自然と表出してきたのです。極端に言えば、100行のコードでレスポンスを0.5秒早められるとしても、実装の複雑さを嫌って導入を見送るような文化が今の我々にはあります。
このバランスの取り方は一概に良い悪いを決められるものではなく、チームの求めるものに応じて適切なポイントを見つける必要があると思っています。
明日は @ikaiman さんがなにか書くようです。余談ですが私は来週も登場予定です。