レビューガイドライン(Review GuideLine)
ここで述べているレビューはピアレビューについての方法です。
（作業成果物の欠陥と改善の機会を探すレビュー）
「最悪を最初に」を基本としてレビューすべき、
たとえば、仕様やアルゴリズムに欠陥があるのに、typoにこだわってもしょうがないので、なにが最悪かを考え、それを防ぐための物からレビューをします。
誤りがプロダクト全体に影響し、手戻りのコストが高くつく、あるいは失敗するようなリスクがないかを考慮にいれてレビューの対象を選択します。
たとえば、基本的な初期フェーズの要求仕様や、クリティカルな決定の基礎になる仕様、使用頻度が高いモジュールなどを重点的にレビューします。
以下に書く項目はレビュアーに負担をかけないようにするのが前提なのでレビュアーに出す前にそもそもテストしたい項目です。
参考：
あなたのおっしゃるレビューってどのことかしら？
コードレビューの際に気をつけること
実際に試したのか、証拠としてスクリーンショットを撮ることも大事です。
デメテルの法則、YAGNI、DRY、KISS、GoFを意識して実装、レビューすることも大事です。
参考：
何かのときにすっと出したい、プログラミングに関する法則・原則一覧
最近だとGoogleもコードレビューのガイドラインを出しています。
Google Engineering Practices Documentation
コードレビュー 開発者ガイド（日本語訳）
最重要チェックポイント
- バグや設計変更があった時に、大変になるところを重点的に見る
- モデルの修正は大変なので念入りに見る
- メールの送信など送ってしまったら取り返しがつかないものはきちんと試してみる
- セキュリティの問題を抱えやすい場所は注意する、ユーザー入力を埋め込んでる場所など (XSS/SQLインジェクション etc)
- サービスの根本的な価値や、お金につながるような変更は死ぬ気で見る
基本的なコードレビュー
- 関数引数の存在チェック(undefined, null, nil)
- 境界条件でうまく動くか
- 結果を返し忘れたりしてしまうコードパスがないか
- 計算オーダーが短く書けないか、DB検索クエリ条件で解決できないか
- 並列処理は同期や整合性が取れているか
- キャッシュを利用している箇所の場合、キャッシュ有無で両方問題ないか
- オブジェクト参照してる変数を上書きして意図しない挙動になっていないか(必要に応じてディープコピーする)
- 処理系依存の処理がないか（await忘れ、C++だとvolatileなど）、どの環境でも実行順番で意図した挙動になっているか？
- マジックナンバーは極力使わない。使ってもコメントで意味意図が書かれているか
- 処理が複雑な場合、仕様の説明がコメントで書かれているか
- 1つのメソッドにやたらとif文のネストある構成になっていないか
（早期return, continueできないか？） - 名前の付け方が悪くて誤解を生みやすいものがないか
- デッドコードがないか
- 無駄な処理をしていないか
- CSR、SSRに問題ないか（SPAの場合）
- AMPの場合に問題ないか（AMPを実装している場合）
APIのレビュー
- APIパスが間違っていないか（他のAPIパスを上書きしていないか）
- 外部サービスのAPIを呼び出す際はエラー処理をしているか
- リクエストパラメータの存在チェック(undefined, null, nil)をしているか
- リクエストパラメータに意図しないデータが入ってこないか(validate)
- トランザクションの処理はデッドロックが発生しないか
- API単体テストが書かれているか
- メール、メッセージ送信系（SMS、LINE）は実際に送信して、開封したメッセージがさらにリンク切れでないか確認したか
- 課金周りの処理は計算が正しいか、エラー時にフロントエンドでの処理も表示できているか
- 決済処理が二重決済にならないか？二重決済や不正決済を防ぐようにCSRFトークンなどで対策できてるか
- GoogleBotなどは集計処理から弾いているか？
DBのレビュー
- DBクエリエラーになるケースがないか（クエリによってエラーになるケースがないか網羅できているか）
- 変なデータが混入するケースがないか（モデルvalidateしているか）
- 複数のAPIの条件分岐に影響するケースがないか
- セキュリティ的に問題のあるパラメータが渡ってこないか（XSS、SQLインジェクション）
DBフィールドの変更、削除に伴う修正は実データをlocalhostのDBに落としてきて必ずマイグレーション確認する（実データは意図しないデータが入っていることが多い）：
- モデルのマイグレーションが発生する場合、フォールバックの処理が書かれているか
（リリース後、マイグレーションスクリプトの実行が必要なケースでリリースの段階を分ける必要がある） - indexを追加、変更、削除する場合問題ないか
フロントエンドのレビュー
- ユーザが操作不能になるフロー、導線はないか
- JavaScriptエラーがないか
- リンク切れがないか、リンクが正しいか
- ブラウザ依存の実装はないか、マルチブラウザ対策できているか(Can I useで確認)
- SPAの場合は、他のルーティングを上書きしていないか
- submitボタンがダブルクリックで二重送信してないか
- APIのパスが間違っていないか
- 正しいデータが取得できているか
- APIエラーの場合の動作が実装出来ているか
- ユーザ入力データを表示する箇所はXSSのサニタイズができてるか
ウェブデザインのレビュー
- データ取得系コンポーネント（リスト等）の場合、データが空の状態の表示（もしくはローディング表示）は実装できているか
- 各種対応ブラウザChrome, iPhone Safari, IEで表示確認できているか
- 画面サイズを変更してレイアウトが崩れないか
- 表示されている画像のサイズは適切か
- 余白が揃っているか
- 文字サイズが揃っているか
- カラーコードに準拠しているか
可読性の良いコードを書く
古くからの名著にリーダブルコードという書籍があります。
いかに読みやすいコードを書くかのノウハウとなります。
まだ読んでない人は一読しておくことをおすすめします。
- 命名規則を統一する。タイポ、表記ゆれをなくす
- 処理を関数化する、意味のあるコードの塊で改行を入れる
- 複雑な条件分岐を書かない（早期return、continueを心がける）
ドメイン駆動開発
さらに発展した考え方にドメイン駆動開発（DDD）というものがあります。
システム要件のコンテキスト（役割）に合わせてモデル（テーブル）やフィールドを作成することで仕様変更や拡張しやすい設計にします。
参考：
役割駆動設計で巨大クラスを爆殺する
関心の分離を意識した名前設計で巨大クラスを爆殺する
CRUD（Create Read Update Delete）に合わせたAPIパスにする
モデル単位のCRUD操作のAPIパスを変更するのではなくHTTPメソッドを変更することで対応します。（無駄にAPIパスの種類を増やさない）
- Create: POST
- Read: GET
- Update: PUT
- Delete: DELETE
例えばuserモデルのCRUD操作の場合
Create:POST /user ユーザ作成
Read:GET /user ユーザ一覧取得
Read:GET /user/:id ユーザ取得
Update:PUT /user/:id ユーザ更新
Delete:DELETE /user/:id ユーザ削除
のように作成します
参考：
綺麗なAPI速習会
テストの自動化
一般的に動作確認の手動テストを行うことは当たり前だと思いますが、
それに加えて、以下のテストを追加することでよりシステムの堅牢性が増します。具体的には回帰テストです。
これがなぜ大事かというとライブラリのバージョンをアップグレードしたり、仕様変更した際に他の箇所と不整合（デグレ）がないか検出することができます。
Circle CI、Travis CI、JenkinsなどのCI(Continuous Integration)を使うことでgit push時に継続的にテストを実行することができます。
- API単体テスト
- E2Eテスト
- ビジュアルリグレッションテスト
APIの単体テストにはJest（NodeJSの場合、これにsinon、supertest、nock、rewire、proxyquireなどのモックライブラリを組み合わせる）を使ったりします。
プロダクトのコア機能をテストするE2EテストにはJest＋puppeteerを組み合わせたりします。
UIの回帰テストであるビジュアルリグレッションテストはstorybook+BackstopJSで行ったりします。
テストカバレッジ
codecovを使うことでテストコードの全体カバー率を見ることができます。
コードカバレッジを１００％に保つ必要はないですが、システムのコア機能のカバレッジはテストの抜け漏れを防ぐためには有効な指標です。
（ただし仕様漏れの場合、実装側で抜け漏れが発生するのでカバレッジが高くてもテストの方でも抜け漏れとなります）
LINT
lintはプログラミング言語の文法チェックです。ウェブサービスの場合はESLintで文法チェックのルールを指定することができます。
またhuskyを使うことでgit commit時やgit push前にlintコマンドを実行させることができます。
Androidの場合はAndroid Lint、XCodeの場合はSwiftLintなどがあります。
レビューの自動化
dangerを使うことで、PRの特定の条件で警告を出したりできます。
- １回のPRコードの行数が５００行を超えてる
- テストコードがないときに警告を出す
- 特定のファイルに手を加えた場合に警告を出す（ORMファイルの修正をした場合など）
これによりレビュアーがどこに気をつけてレビューすればよいのかわかりやすくなります。