今日から始めるコードレビュー

ソフトウェアの品質についてちょっと整理したうえで、私の個人的なコードレビューの考え方と観点を上げてみました。

テスト観点とかぶりますが、テストで検証しようとすると意外と大変だと思われるので。

前提として、Railsアプリケーションを意識しているため、Rails固有の用語はありますが、Javaエンジニアをやってた頃に学んだ観点を持ってきているので、言語やフレームワーク依存な考え方はしていないと思っています。

雑なメモになっていますが、徐々に直していきます。

TODO : チェックリスト化していきたい。

システムに求められるものとは

大前提として、システムは必要な人に必要な情報が必要なタイミングで届くようになっているかが重要。

それを是として各種の考え方にブレイクダウンしていくのが良いと思っています。

コードレビューの考え方

コードレビューで見つけるべき欠陥

ソフトウェアテストで発見できない欠陥は、コードレビューで見つけるべきです。

私の感覚だと、機能要件および非機能要件のうち使用性については、ソフトウェアテストでわかる部分なので細かくレビューしません。

（ただし、機能要件でも不明瞭な部分についてはレビュー時点で確認したほうがリードタイムが短くて済むので、レビューで指摘するようにしています）

QA活動としてのレビュー

QAさんは、ソフトウェアテスト手法を駆使して欠陥を発見してくれますが、ソフトウェアテストは万能ではありません。

実際、弊社ではQAさんにも設計レビューに参加してもらったりします。こうすることで、開発前に設計レベルでの欠陥を発見することができ、手戻りのリスクを減らすことができます。

こちらの本のP.81では、QA活動はテスト以外にもあるよと紹介されています。

ちなみにこの本には、P79に「もしこれ以上のバグが出るようならそれは非機能要求のバグですから非機能要求のバグは非常に数が少ないはずなので、あなたが徹夜するような事態にはなりません！」って書いていあるけど、そういうよくわからんバグで徹夜しないけど苦労するのはエンジニアの僕なので勘b(ry

コードレビューの観点

コードの意図を理解する

１行ずつ、頭の中でコードを実行し、なぜこのコードが必要なのかを考える。
説明できないコードについては質問する。

特にトリッキーなコードやイレギュラーなコードについてはそれを重点的に行う。

説明がついたら、「なぜ必要なのか」が暗黙知にならないように、コードにコメントを残してもらう。

合理的な説明ができなければ設計の再検討が必要な兆候なので、相談、ないしは書き直してもらう。

KISSであるか

KISS原則も参照のこと。

とにかく簡素であること。DRYやYAGNI などのほかの原則はいったん忘れる。

いろんな原則を頭においてレビューすると、判断基準が多くなって迷いがでるので、コードの書きっぷりについては、簡素であるかだけを考える。

KISSを指摘する過程で、重複コードを集約するとシンプルになるよね、という指摘が出てくるので、ほかの原則はいったん置いておいても心配ないと考えられます。

また、不要なコメントやコードは削除することも、コードをシンプルにする上では重要です。

想定されるシナリオにおいて、正しく動作するか？

特に並行処理したり、多くの件数のデータを処理した場合に正しく動作するかを検討する。

計算のオーダーとか。

実装は変更に耐えられるか？

前提として、仕様通りになっているかを確認する。

そのうえで、将来起こりえる変更を予想する。変更に対して対応できるかを検討する。

その対応の中で不吉なにおいがないかを嗅ぎ分ける。

もし不吉なにおいがあれば、パターンや原則の導入を検討する。

使用するフレームワーク・ライブラリのお作法に従う

知らないフレームワークやライブラリが使われている場合、ドキュメントやプラクティスを調べながら、使い方が正しいことをレビューする。

社内標準とか、どこかの部署に筋を通しておけとかも確認しておくこと。

名前重要

名前重要 も参照のこと。

クラスやメソッドの責務を表す名前になっているかを確認する。ほかのリファクタリングは後回しでよいので、時間をかけてよい名前を検討する。

ここが適当だと、責務があいまいになっているので、のちの改修の際に処理を追加する箇所の判断がむつかしくなってしまう。

あとで適切な名前を付けようとしても開発された時のコンテキストが失われていると、適切な名前を考えるための情報が集められないかもしれません。

そのため、技術的負債として一番返済がむつかしいものだと考えています。

ロギング、例外

ソフトウェアテストで見逃しがち、かつ静的解析ツールでもチェックされにくい部分は、設計の領分である。

これらは運用フェーズになった時にしわ寄せがくるので、最低限抑えるべきところがしっかりしているかを見ておく。

特に、エラー発生時にエラーの追跡が可能なようにメッセージが記録されるか、通知が行くかなど。

あと、運用監視ツールの設定どうするの、とか。Sentryなどの例外監視ツールへの通知はどうするか。

ログレベルが適切か

Railsガイドの2.5 ログがパフォーマンスに与える影響 も参照。

debugログを出力するときに、オブジェクトを文字列に変換する処理だけ実行されて、実際にはログ出力されない、みたいなことが起こりえる。

これがパフォーマンス劣化の原因になることがあるので、書き方に注意する。

パフォーマンス上の問題はでないか？

SQLはインデックスを使うようになっているか？

SQLで処理されるデータの件数は多すぎないか？多い場合は非同期で処理するようにできないか？

キャッシュを使うようになっているか？

SQLのトランザクション分離レベルは？

可能な限り分散トランザクションを使わないようにしているか？
分散トランザクションを使うと、システムがスケールしなくなるし、SPoFができたりとあまり良いことがないので。

キャッシュの設定は適切か？

並行性に関する不具合にも関連しますが、パフォーマンスの話題なのでこちらで扱います。

参考1：CDN切り替え作業における、Web版メルカリの個人情報流出の原因につきまして

参考2：1.3フラグメントキャッシュ (Rails のキャッシュ機構)

参考3：特異なバグ

キャッシュの有効期限やキャッシュのキーは適切か？

複数ユーザで同時にアクセスしたときに、意図しないキャッシュが表示されないか

並行性、トランザクションを用いた個所はあるか？

参考1:スターバックスは2フェーズコミットを使わない

参考2： マイクロサービスにおける決済トランザクション管理

参考３：メールのトランザクション設計

並行性やトランザクションに関する不具合はないか？

そもそも、これらの実装はDBやフレームワークなどに押し込めるべき。あるいはステートレスにしたり冪等性のある仕様にするべき。

それらの選択肢を検討したか？

どうしてもそれらを実装しなければならない場合は、並行処理に関する書籍を読みながら頑張ること。

特にトランザクションをサポートしていないリソースをトランザクションに参加させる場合、補償トランザクションの実装をしなければならないので、可能な限り工数がかかるとリジェクトするべき。

あるいは、メールのようなトランザクションに参加できないものの場合、DBのトランザクション内で処理するようにして、データの不整合が起きにくくする。

データの整合性が重要な場合、データを突合して矛盾があったらアラートを上げる仕組みを用意する必要がある。

システム連携は存在するか？

連携する場合、連携先のシステム側のリリースの単位を確認すること。
リリースしましたと言われた時、何をリリースしたのか詳細を聞くこと。

システム連携をする場合、データの更新に成功したかどうかをどう判断するか？

連携先のシステムの更新に失敗した場合、リカバリーはどうするか？

システム障害などで、連携を一時停止したい場合、どうするか？

連携先のシステムには、キャパシティの制限や、API呼び出し上限などは存在するか？（SFのガバナ制限など）

リリースについて

デプロイ手順はどうするか？ 複数のシステムのリリースを行う場合、順番はあるか？

切り戻し手順はあるか？

PRや物件の依存関係はどうなっているか？
特に複数のPRをリリースする場合、PR同士が矛盾する仕様を持っていたりしないか？
一報のPRは古い仕様のまま開発を進めていて、もう一方のPRの変更を取り込み忘れていないか？

（追加）

文字コード

[改訂新版]プログラマのための文字コード技術入門 (WEB+DB PRESS plusシリーズ) を読んで、文字コードの取り扱いについて不味そうなところを指摘する。

例えば、JISコードの「髙」（はしご高）、SJISの「〜」、 utf8mb4 における「槗」が正しく扱えるかがハマりどころ。