このドキュメントについて
レビューポイントっていうのは周囲のレベルや使っている技術によりまったく異なるのですが、割と汎用的に使えそうな部分について今の開発チーム向けに書いたドキュメントを共有
レビューチェックポイント(機能)
- 仕様と実装が合っているか
- 仕様が不明瞭な部分について勝手な仕様で実装をしていないか
- 違和感のある実装になっていないか
- ロジック・アルゴリズムが合っているか
- そもそも論理構造が破綻していないか
- 画面表示に違和感がないか
- 奇妙な文言、ユーザ視点で奇妙な動きになってないか
- 不正なユーザ入力で問題が発生しないかFatalErrorにならないか
- 入力に対し適切なvalidationが行なわれているか
レビューチェックポイント(コード)
- 意味の無いロジックが含まれていないか
- コピペでコードを持ってきた場合にコピペ元の不要なコードが入っていないか
- そもそも基本コピペはNG
- コピペで持ってくる場合には論理的に意味の無いコードは消すこと
- 不要になったロジックが残ってしまっていないか
- 後で見た人にとって謎のロジックが含まれてしまうので不要になったらちゃんと消すこと
- 同一のロジックが2箇所以上で書かれていないか
- Don't repeat yourself(DRY)原則を守ろう
- 複数箇所に同じ修正を入れ続けなければいけない苦痛を考えると同じ処理は一カ所にまとめた方がいい
- ただし特定期間中しか使わないコードなど保守されずに書き捨てられるようなコードなら無理に守らなくても良い
- コメントが適切か
- 第三者が見た時に理解できるコメントになっているか
- 特殊なロジックが入っている場合には何故そうしたのかについてコメントで書かれているか
- コードに何か使用上の注意点がある場合にはコメントに注記する
- 関数が何をInputとし、何をOutputとするのかについて適切に書かれているか
- ArrayやJson Objectを使っていて返す構造が不明瞭な場合にはコメントにどのような形式で返ってくるのか明記する
- 第三者が見た時に理解できるコメントになっているか
- 命名が適切か(変数名・メソッド名・定数名など)
- 他のモジュールで使われている名前と別の名前になっていないか
- できるだけシステム内での統一言語(ユビキタス言語)になることを意識する
- 他のモジュールで使われている名前と別の名前になっていないか
- メソッド名が適切か
- メソッド名から返ってくる値が予測できること
- getXXXMessage()の返り値でintegerが返ってきていたりしないか
- メソッド名から返ってくる値が予測できること
- メソッド名から想定しづらいような副作用が存在しないこと、もしくはドキュメント化されていること
- getメソッド中でsetをしているとか
- 単体テストが書かれているか
- 正常系のテストが書かれていること
- 容易に起こり得る異常系のテストが書かれているか
レビューチェックポイント(インフラ・パフォーマンス)
- 指定されているデータ型が適切か
- Databaseがレプリケーションをしている場合slave遅延が考慮されているか
- 同時に複数端末で実行して不正な状態にならないか
- 複数のリソースを同時に変更する場合にはロックをかける必要がある
- トランザクションが使えるシステムならトランザクションを使用する
- 重い処理が入っていないか
- 以下の条件にあてはまる場合には改善が必要
- メソッドレベルで200msec以上かかる
- HTTPリクエストレベルで1sec以上かかる
- 改善策
- 計算結果をキャッシュに詰めて2回目以降はそれを使用する
- 外部リソースへのアクセスは一回にまとめて実行する
- 100回insert文を実行するよりも100レコード文のbulk insertの方が速い、など
- 以下の条件にあてはまる場合には改善が必要