概要
実際にレビューで指摘した事項についてまとめる。
実務でどのような「好ましくない」コードが作成されるのか、
という情報を元にレビューの視点や規約化などに活かすことを目的とする。
随時更新
Javaのレビュー指摘
変数名関連
※頻度は指摘頻度です。
| 頻度 | 指摘事項 | 詳細 | 追加日 |
|---|---|---|---|
| ○ | 不適切な変数名 | リンク先参照 | −− |
| × | 変数の内容に依存する変数名の利用 | エラーコードそのものを変数名にしたものなど。エラーコードが変更されたら変数名と不整合が発生する | 2012/09/18 |
パフォーマンス
| 頻度 | 指摘事項 | 詳細 | 追加日 |
|---|---|---|---|
| △ | break漏れ | ループ処理でbreakが必要な箇所でbreakせず毎回全件ループしている | 2012/12/31 |
| △ | Mapの利用 | 2つのリストをキーを下に比較するような処理で順次比較してしまっている。Mapのキーで比較する処理に置き換える | 2012/12/31 |
| ○ | ループ内の過剰処理 | ループの外に出せる処理がループ内にある | 2013/04/15 |
Javaの作法関連
| 頻度 | 指摘事項 | 詳細 | 追加日 |
|---|---|---|---|
| △ | インターフェースを利用していない | HashMapやArrayListを実装で宣言している | −− |
| △ | Setの利用 | Setを利用すれば済む場合に箇所でHashを利用している。 | −− |
| △ | static宣言 | static finalで宣言出来る定数などを通常の変数として宣言している。static finalに変更 | 2012/09/25 |
| ○ | Javaのコンパイル警告を気にしない | 警告が出ていても気にしない人。これ、かなり多い。放置していると警告量が激増して問題が埋没するので必要性を継続して説くこと | 2012/09/25 |
| × | 0,1を真偽判定に使わない | 0,1を真偽判定に使用せず、booleanを利用する。C言語等が主言語の人にありがち | 2012/09/25 |
| × | doubleの使用による桁落ち | 小数点を含む演算でdoubleを利用して桁落ちする。BigDecimalを利用すること | 2012/10/28 |
| ○ | リストの初期サイズ未設定 | リストの初期サイズを設定する際に、設定件数が予め分かる場合は初期サイズを設定する。デフォルトは10 | 2012/12/31 |
| ○ | 不要なnew | 変数宣言時にnewしているが、そのあとすぐに値を代入する場合など初期化した値を全く利用していない場合。newの部分を削除する。 | 2012/12/31 |
| △ | 例外を例外以外の目的で利用している | 数値や日付不正のチェックなど、例外をあえて発生させて例外以外の目的で処理している | 2013/04/15 |
| ○ | 例外を隠蔽している | 例外時に、何も処理していない | 2013/04/15 |
ヒューマンエラー
| 頻度 | 指摘事項 | 詳細 | 追加日 |
|---|---|---|---|
| ○ | コピペの残骸 | どこの現場でもよくある。コードの流用は構いませんが、自分で全ての行を説明出来る状態にして利用してくださいと話しています。 | −− |
| ○ | 規約違反 | 規約を守らない。CheckStyleなどではチェック出来るものは対応できるがその他はどうにもならないか。 | −− |
| ○ | 手作業を減らす | コード補完、リファクタリング機能を利用しなかったことによる作業ミスによるバグを減らすためにEclipseのショートカットを教える | 2012/09/18 |
| ○ | 不要なimport残 | 根強いミス。EclipseのCtrl+Shift+Oを手癖にするように粘り強く指摘している | 2012/09/25 |
コメント関連
| 頻度 | 指摘事項 | 詳細 | 追加日 |
|---|---|---|---|
| ○ | 過剰なコメント | 説明がなくても意味が取れるような処理に対してコメントは不要。 適切な命名を行えば大多数の処理はコメントが不要になるはず。 パフォーマンス対応、既存システムの流れで変更出来ないメソッド名や変数名の影響など 特別な事情がない限りコメントは不要。 コメントで補足せずに、可能な限り変数名やメソッド名の命名で可読性を高めること。 |
−− |
| × | 連番付きコメント | 詳細設計書に行レベルの設計がある場合などに見かける。設計書の連番とソースコードのコメントの連番を同期させてしまっている。変更のコストが大きく、よりよい設計への障壁が高まる。また適切な命名がなされていれば不要なコメントになる | 2012/09/18 |
| △ | コード値つきコメント | Enumや定数のコード値をコメントに書いてしまっている。「Web:1の場合・・・」など。定数値が変更になった場合にコメントのメンテも必要になるので非推奨 | 2012/09/18 |
テスト関連
| 頻度 | 指摘事項 | 詳細 | 追加日 |
|---|---|---|---|
| △ | テストのためのテスト | 処理を検証するためではなく、テストを成功させるためだったりカバレッジを上げるためだけのテストコードを書く。 これにdjUnitのVMockの誤使用が加わると恐ろしいテストコードが出来上がる |
2012/09/25 |
| △ | assert文のないテスト | Unitテスト自体を理解していない開発者がやりがちなパターン。何もしない人と標準出力してテストした気分になる人の2種類がいる。 対応策はUnitテスト自体について基礎から説明すること |
2012/10/01 |
言語を問わない一般的作法
| 頻度 | 指摘事項 | 詳細 | 追加日 |
|---|---|---|---|
| ○ | マジックナンバー | どこでもよく見るPart2。マジックナンバーを定数にしてくださいと言ったらマジックナンバーの定数名にMAGIC_NUMBERと命名した猛者が居た。 | −− |
| △ | 重複したコード(＝WETなコード) | 同じ処理を共通化せず何度も書く。共通化を行う。 | −− |
| △ | 長いメソッド | コーディング規約で長さの規定を作成し、CheckStyleで検出することで問題を発見する。長すぎるメソッドがある場合、共通化やComposed Methodによる処理の分割を検討する。 | 2012/09/18 |
| △ | 多すぎるメソッド数 | コーディング規約で数の規定を作成し、CheckStyleで検出することで問題を発見する。クラス分割の検討をする | 2012/09/18 |
| × | 全てのステートメント後に改行がある | 論理的なブロックごとに改行を開けて処理の意味を持たせたい場合に、全ての行が改行されてしまっていると2行以上開ける必要が出てきてしまう。また全般的にコードが縦に2倍伸びるためエディタ内でメソッドを読む際にスクロールする必要が出るケースが増える | 2012/09/18 |
| △ | 共通処理の未使用 | 予め用意していた共通処理ではなく、リファクタリングしながら作成したような共通処理が新たに作成されたソースに適用されない場合がある。JavaDocは残してあるのだが全てに目を通してはもらえないし、レビューで炙り出すのが正解なのだろうか？ | 2012/09/25 |
| × | 行間を作成し、意味を持たせていない | 全ての処理で改行が全くない。処理の意味区切りごとに改行を入れて読みやすくすること | 2012/10/28 |
