コードレビューについて

最近、コードレビューをすることが多いので、大体何を見ていることが多いのかを書きます。

基本的なプロジェクトルールと、レビュアーとレビューイの定義

前提として、プロジェクトルールと、レビュアーである私とレビューイがどんな人なのかを述べます。

プロジェクトルール

• テストコードを実装の上、それが全て通っている段階でレビュー依頼をすること

レビュアー（私）

現在勤めている会社では人事・労務領域のアプリケーションを作成しています。およそマスタ（組織・社員など）および業務でモジュールとして切り出していて、それらに一人の主担当がついている構成になっています。

私もいくつかのモジュールの主担当をしていて、それらの機能についてはロードマップの検討・設計・実装・リリースを行っています。そして、さすがに1人では実装までやってられない時はメンバー等に実装をお願いするわけですが、品質担保のためコードレビューを行っています。

ここでのレビュアーというのは以下の性質を持っていることになります:

• そのモジュールに対して、特権的な立場にある
  • 設計を変更することができる
  • そのモジュール開発におけるルールを作ることができる
  • コードがマージされるというのは、「私が承認した」と一致する
• そのモジュールに対して、品質担保責任を負う

レビューイ

そして、実装担当者は以下の性質を持っています:

• 同じチームのメンバー (新卒1年目〜2年目程度)
  • 容易に同期コミュニケーションができる。
    • この会社は全員出社でコアタイムがあるので、周辺に座ってます。
• オフショア会社のメンバー（スキルレベル等不明）
  • 日本語がわかる人が僅かにいる。
  • 実装担当者が日本語を分からない場合がある。その時は設計やレビューが伝言ゲームされる
  • テキストでの非同期コミュニケーションをとらざるをえない

レビュー観点

大体以下のようなことを見ています。多分よくある「可読性の高いコードを書こう」みたいな本に載っているようなことくらいしか見ていないと思います。

• 1.0. 現在のスタイルに従っていること
  • 1.1. クリーンアーキテクチャにしたがっていること
    • 1.1.1. コントローラ層のクラス (Request, Response) がサービス層でインポートされていないこと
    • 1.1.2. リポジトリ層のクラスがサービス層でインポートされていないこと
  • 1.2. 他のコードとスタイルが合っていること
• 2.0. 設計意図に従った実装であること
  • 2.1. 設計に対応するテストが実装されていること
    • 2.1.1. そのテストが通過していること
  • 2.2. 設計に対応するような関数が実装されていること
• 3.0. 認知的負荷が十分に少ないこと
  • 3.1. 同一ブロック上で定義されている変数が十分に少ないこと
  • 3.2. 妥当な命名がされていること
    • 3.2.1. 寿命の長い変数名は十分に説明的な名前を持っていること
    • 3.2.2. 関数名が適切であること
  • 3.3. 余計な変更を行わないこと
    • 3.3.1. 「オブジェクトを組み立てて返す」ということが目的の関数で、結果用オブジェクトに対して行うものをのぞいて、オブジェクトの値を書き換えないこと
• 4.0. パフォーマンス・セキュリティ的な懸念がないこと
  • 4.1. フォーム入力文字列でクエリを直接組み立てるようなことをしていないこと
  • 4.2. 大量のクエリが発行されていないこと
  • 4.3. 大量のデータを格納していないこと
• 5.0. 他の機能に対する横展開が必要か / 新規設計・設計変更等が必要か

よく指摘すること

現在勤めている会社の新人である場合は スタイルと認知的負荷 のところの指摘は多くなりがちです。こういう時に読みづらくなるよという話をして納得いただいている感じです。

オフショア開発会社のコードレビューで苦しんでいるのも現在のところは認知的負荷のところになっています。変数名が key だったり item だったり、多分実装担当者はその業務が何かわかっていないので機械的な命名をする他ないのかなと思って見ています。

私の望み

「設計意図に従った実装である」というところ、これが結局、機能の本質であるところですから、基本的にはこれ以外はレビューしたくないんですよね。

叶うなら全てのメンバーに以下のようなリソースは読んでいてもらいたいところ...

• 「綺麗なコード」に関するもの
  • リーダブルコードとかコードコンプリートとか
• 「リファクタリング」に関するもの
• 「認知的負荷」という概念に関するもの
  • A Philosophy of Software Design とか
• 「ドメイン駆動設計」に関するもの

私も別に読みやすいコードを書けるわけじゃないし今でも「なんだこのコード！」って思ったものが自分のコードだったりするので、常に意識して実装していきたいところです。

以上！