---
title: Javaのソースコードレビュー観点をまとめてみた
tags: []
categories: ["Programming", "Java", "CodeReview"]
date: 2012-06-17T12:29:25Z
updated: 2012-06-19T09:58:10Z
---
とりあえず一般的なアプリ向け。Webとか特定FWに特化するともっとある。セキュリティに関しては中途半端に書くと網羅感がないので、いったん抜きで。
普段意識していることを品質特性ごとに思いつく内容をメモっていく。
階層の違うやつは指摘例。
つっこみ歓迎。
### 機能性
- ログのレベルが適切か
- 内容的にはWARNログなのにdebugメソッドで実行されている
- 入力チェックが行われているか
- 想定外の入力の対応が漏れていないか
- NullPointerExceptionが発生しないか
- nullチェックしてください
- 仕様上nullは返らないのでnullチェックの必要がありません
- ClassCastExceptionが発生しないか
- タイプセーフにしてください
- ClassCastExceptionは発生しえないので、チェックの必要はありません
- 設定ファイルが間違っていないか
- 他モジュールとの整合性は保たれているか
- 連携先が異常な場合の対処がとられているか
- 想定される例外に対処しているか
- 例外のハンドリングが適切な箇所で行われているか
- システム例外処理を集約してください
- ディープコピーすべき箇所でシャローコピーが行われていないか
- セキュリティ脆弱性がないか
### 信頼性
- リソースを必ずcloseしているか
- close処理がfinallyで実行されていない
- 一貫性を保つべき処理がAtomicになっているか
- Map#containsで存在チェックしてからputしているが、putする前に他スレッドからputされる可能性がある
- トランザクション範囲は適切か
- そこでコミットされて大丈夫ですか
- メモリリークが発生していないか
- キャッシュ機構を用意して、増え続ける一方である。消去する方法、タイミングを提供すること。
- スレッドアンセーフになっていないか
- staticなフィールドにHashMapを使用せず、ConcurrentHashMapを使用すること
- immutableなフィールドにfinalがついているか
- スレッドプール数、ワーカー数は妥当か、外部化されているか
- 取得するデータ量が多くてOutOfMemoryErrorにならないか
- 一度に全件メモリに確保せず、RowHandlerなどでカーソル処理を行うこと
- 他モジュールからアクセスされた場合に不整合にならないか
### 使用性
- 汎用的な用途に対して、十分な拡張性を提供しているか
- 処理をハードコードせず、Strategyパターンを使用し、処理をプラガブルにすること。
- APIの外部仕様は明確か？
- 返り値や引数にnullは取りうるかやスレッドセーフかどうかが明文化すること
- クラス名、メソッド名が直観的であり、処理の内容を想起できるか
- Runnable#runをAPIとして公開せずexecute等わかりやすいメソッドに委譲して公開すること
- パッケージ名は適切か
- 処理内容がメソッド名を逸脱していないか
- startメソッド内でstopメソッドを実施しない
- 不要な暗黙のルールを設けていないか
- 設定項目の外部化がおこなわれているか
- 設定ファイルが多すぎないか
- 単体テストしやすいか
- スロー宣言しているチェック例外が、適切にハンドリングしてほしい場合のものか
- スローする例外が限定的すぎないか
- 例外クラスの選択は正しいか
- NoSuchElementExceptionの用途が違う
- メソッドのjavadocをみて入力と出力が分かるか
- クラスのjavadocをみてそのクラスがどういう役割を担っているかわかるか
### 効率性
- 正しいデータ構造を使用しているか
- 順序性を保つ場合はLinkedHashMapを使用すること
- ログレベル確認のif文をいれることなく、ログを出力するパラメータの構築にメソッド呼び出ししていないか
- O(N^2)のアルゴリズムを使用していないか
- 一度行えば十分な処理(入力チェックや初期化等)が何度も実行されていないか
- キャッシュを使用できないか
- コレクションのサイズが想定できる場合に、そのサイズで初期化しているか (＊1)
- 文字列の連結がStringBuilderで行われているか(コンパイラが行っている場合はOK)
- StringBuilder/Bufferの長さがおおよそ分かっている場合はそのサイズ（大きめ）で初期化しているか
- String#matchesが繰り返し実行されていないか
- Pattern#compileの結果をフィールドにもって使い回すこと
- 自前のループで配列をコピーしていないか
- System#arraycopyを使用すること
- InputStreamからOutputStreamにデータをコピーする際、1バイトずつコピーしていないか
- 不要なオートボクシングが発生していないか
- SQLの発行回数は適切か
### 保守性
- APIを正しく使用できているか
- 同じ処理が重複して実行されないか
- ほとんど同じ処理がコピペされていないか
- 処理を共通化すること
- クラス設計が妥当か
- 拡張for文が利用できる箇所で、通常のfor文、while分を使用していないか
- 複数回利用される文字列がハードコードされていないか
- 定数にすること
- Genericsが利用できる場面で使用しているか
- タイプセーフか
- Collectionを使用すべき箇所で配列を使用していないか
- 定期実行する際にポーリングループではなく.ScheduledExecutorServiceを使用してるか
- 処理の待ち合わせにポーリングループではなくCountDownLatchなどのバリアを使用してるか
- 意味のある固定値でEnumを使用してるか
- 利用しているjarのバージョンは適切か
### 移植性
- 特定の環境を前提としていないか
- ライブラリの内部クラスを使用していないか
- sunパッケージを使用していないか
＊1 ... HashMapの場合は負荷係数(デフォルト0.75)もあり、サイズの初期値=最大値にならないように注意
----
↓の本は必読
