汚いプログラム(ものによってはバグを引き起こすプログラム)にするためにはどうすればいいのかを書きました
- 基本的にJavaをターゲットにしています(ものによってはJava以外でも当てはまります)
- 私が今まで、読みにくい、修正しにくい、プログラムの挙動が読めないと感じたソースをもとに抽出した内容を記載しました
- 以下の逆をやれば、ある程度ソースがきれいになると思います
メソッド
- メソッドの引数の数を多くする
- メソッドの引数に巨大なオブジェクト(いわゆる神オブジェクト)を渡すようにする
- 1メソッドを長くする
- メソッド内のネストを深くする
- ガード節が導入されていないif - elseを用いる(早期returnをしない)
- 重複した処理を存在させる
- 配列とListなどを混在させた整合性のとれていないメソッド群を提供する
- 同じ型の引数を3個以上とるメソッドを定義する(例えばint型のパラメータを3つ渡すメソッド)
- 各引数が何を意味しているのか分かりにくくなる
- メソッドの呼び出し側のコードを読むと、メソッドに与えている引数が正しいのか間違っているのか判別しにくい
- ローカル変数の無駄な初期化を行う
List<String> list = new ArrayList<>(); list = findAll(); // 上で一度初期化しているが、findAllの結果を代入しているので初期化が無駄になっている ・・・ return list;
nullの扱い
- nullをフラグの扱いにする
- Nullオブジェクトパターンで回避できる
- メソッドの引数にnullを渡すことを許容する
- 戻り値の型がListのメソッドにおいてnullをreturnする
- インスタンス変数へのnull設定を許容する
例外の扱い
- メソッドがjava.lang.Exceptionをスローする
- キャッチした例外を握りつぶす
- NullPointerExceptionをキャッチする
- バグを意味するエラーを、チェック例外(検査例外)を使ってスローする
- 一つのメソッドから複数の例外をスローする
- CloneNotSupportedException や UnsupportedEncodingException が発生した際に上位にそのまま伝播させる(スローする)
- これらの例外はチェック例外だが、キャッチしてもできることはほとんどない。(これらの例外が発生するのは環境不備やバグに近い)
- デフォルトコンストラクタで生成した(エラー発生時のコンテキスト情報を一切含めない)例外をスローする
- 詳細なエラー情報が無いのでcatchした側では何が原因で発生したのか理解できなくなる(ただし、例外のクラス名自体が説明的である場合は必ずしもこの限りではない)
スコープ制御
スコープを広げて余計なものを見せると、不要な関係性ができて結合度が高くなってしまい、分かりにくいプログラムになっていく。
(スコープ = 見える範囲 = 依存する範囲 = 保守性に影響を与える範囲)
- あらゆるクラスをpublicにする
- クラスに限らずスコープが広すぎることは不要な関係性を持つことにつながる。スコープは狭ければ狭いほど扱いやすい。
- コンストラクタも全てpublicにする必要は無い。見えすぎると利用者に余計な選択肢を与えてしまう
- あらゆるメソッドのスコープを広げる(例えばどんなメソッドでもpublicにする等)
- あらゆるインスタンス変数やクラス変数のスコープを広げる
オブジェクトの生成
- インスタンス生成するための機構としてコンストラクタしか用意しない
- コンストラクタ呼び出しでは、どのような意味のインスタンスを生成するのか?という内容を表現しにくい(new XXXX() という記述は表現力に欠ける)。状況によっては、ファクトリメソッドやオブジェクト生成用のBuilderを用意したほうが分かりやすい
- 1クラスにたくさんのコンストラクタを定義する
- 利用者側からは各々のコンストラクタをどう使い分けていいのか分かりにくい
命名
プログラムを書くことの重要な側面として名前付けがある。名前付けをサボればサボるほど、プログラムはどんどん汚くなっていく。
- メソッド名に統一感を持たせない (「open」の対が「stop」になっていたり、 「display」と「print」のボキャブラリーの揺れがあったり等)
- 型を誤解させる変数名にする
- メソッド名を冗長にする(メソッド名にクラス名が入っている)
- 異常に汎用的なクラス名を使用する
- 例えばUtilクラス(Util.java)というものを作ってしまうと、ありとあらゆるメソッドが置ける場所になってしまい、崩壊していく
- クラス名にxxxManagerとつける
- Managerは便利な名前だが、名前が汎用すぎていまいち意味がわかりにくい、多用すると第三者は理解しにくい
- そのアプリケーションが提供する概念的な機能名(の英名)がメソッド名にもクラス名にもパッケージ名にも現れない状態にする
- 概念的な機能のドキュメントとプログラムのマッピング取りにくく、ソースコードが追いにくい
- 対象機能が呼び出されるトリガ(例えば、画面のあるボタンが押された)の処理を起点にして、その機能が紐づくコードがどこかを探し当てる必要が出てくる。非常にわかりにくい
- boolean型の変数にxxxFlagやxxxFlg という変数名をつける
- trueとfalseがそれぞれどういう状態を意味するか分かりにくい。場合によっては、trueとfalseを逆の意味に取り違えてしまう
- 伝えたい意味とは異なった意味の単語を使う
- rawdata とすべき場所で rowdata とつける
- cache とすべき場所で cash とつける
- 存在しない単語を使う
- regist という単語を使う(正しくはregister)
- メソッドの処理内で、目立たなくてよいローカル変数をフルスペルで記述する
- 例えば、Transactionという型の変数を使う場合は、transaction という変数名ではなく tx で十分な場合が多い
- フルスペルで記述してしまうと悪目立ちしてしまう場合がある。適切に略称などを使った方がよい
- メソッド名には必ず動詞をつける
- 昔はメソッド名には動詞をつけるという話もあったが、それに固執すると表現の幅が狭まる。動詞である事に執着する必要は無い。
- スペルミスを放置する
- スペルミスがあると読む側に 間違った名前⇒正しい名前 の変換コストを強いることになります。場合によっては意味を誤解させることもあります。
プリミティブ型
int型はあくまで数値、String型はあくまで文字列であり、それ以上の概念的な意味を持たせるのは無理がある。プリミティブ型に固執すると分かりにくいプログラムができあがっていく。
- メソッドが成功・失敗や何らかのステータスコードを表現する戻り値を返す場合に、プリミティブ型やString型を用いる
- プリミティブ型やString型は表現力が乏しい。適切なオブジェクトを定義したほうが分かりやすい
- 2つの選択肢を持つ値を表現するのに必ずboolean型を用いる
- ON/OFF, Import/Export, Enabled/Disabled 等を表現するのにboolean型ではわかりにくい。(場合により意味を取り違える)
- enumや独自のオブジェクトを定義したほうが分かりやすい
enum
- enumを用いず、タイプを意味するようなint定数を定義する
適切ではないAPIの使用
- Stringを "==" で比較する
switch文
- Typeごとの処理をswitch文で分岐させる
- ポリモーフィズムで置き換え出来ることが多い
- default句を書かない
- default句が無いとswitch文の条件分岐で想定しない値が入った場合の挙動が掴みにくくなる
パッケージング
- 全クラスを同一パッケージにする
- 意味の異なるクラスを同一パッケージに入れる
- 利用者向けのパッケージと実装者向けのパッケージを分けない(api と spiを分けていない)
継承
継承は強力な武器であるが、振り回すのが難しい巨大な武器である。使い方を誤るとプログラムを一気に複雑にする。
- 継承の階層を深くする
- 実装の継承をする
- 複数クラスでコードを共通化させるためだけに継承する
- インターフェースが現れず継承ばっかりする
- 継承されることを予定していないクラスをfinalにしない
- そしていつの間にか意図せず継承されてしまい、親クラスを簡単には変更できなくなる。
インターフェース
- 実装クラスしか存在しない状態にする
- インターフェースではなく具象クラスを戻り値の型にする
- ListではなくArrayListを戻り値の型にする
デザインパターン
- シングルトンパターンを多用する
クラス定義
- クラスコメント(クラスのJavadocコメント)を記述しない
- そのクラスが何者であるかをコメントで宣言しておかないと、機能追加する際にどんどん意図したクラスから外れていきやすくなる
- staticイニシャライザに複雑な処理、長い処理を記述する
- 使用していないインスタンス変数を残す
- 使用していないprivateメソッドを残す
- Mainクラスにmainメソッド以外のpublicメソッドを定義する
- 同じ型の引数を3個以上とるpublicコンストラクタを定義する(例えばString型のパラメータを3つ渡すコンストラクタ)
- メソッドの項で書いたのと同様です
- メソッドと違いコンストラクタには名前が付けられない分、メソッドよりたちが悪い
- 引数違いの複数のコンストラクタが存在している場合は、なおさら理解しにくくなる
スレッドセーフ性
- メソッド間で値をやり取りするための一時変数のようなインスタンス変数を存在させる
- SimpleDateFormat をstaticメンバにする
配列
- 配列の要素ごとに意味を持たせる(たとえば以下のような配列。このような配列を公開されるメソッドの引数や戻り値にすると、第三者は理解しにくくなる)
String[] personData = new String[3]; personData[0] = "Jabao Yamada"; // 氏名 personData[1] = "185-0023"; // 郵便番号 personData[2] = "東京都西国分寺"; // 住所
Collection
- メソッドで、size=0の空のListを返却する処理を return new ArrayList
() と書く - Collections#emptyListを使った方が意味が分かりやすい(ただしこのメソッドを使った場合、対象のListには要素を追加できないという件は理解しておく必要がある)
コメント
- ソースの変更履歴をコメントを使ってソースコード中に残す(もとのコードもコメントアウトして残す)
- コメントアウトして残されたコードは、ソースを読む際のノイズになりやすい。バージョン管理システムを使えばよい。
他システム連携
- 他システムが定義したオブジェクト(クラス)をメソッドの戻り値にする
- 外部システムとの連携で発生した外部システムの例外を利用側内部まで通知する
- 他システムが定義したオブジェクト(クラス)をいろんな場所で使う(外部システムのクラスを知っている責務を色んなクラスやパッケージに負わせる)
- 他システムと直接的にやり取りする責務は、特定のlayerやパッケージ、クラスに限定したほうが良い
他システムに提供するAPI
以下は全て、連携先から呼び出されるメソッドについての内容です(ただし他システム連携に限らず自システム内でも当てはまる内容です)
- メソッドの引数にMutableなオブジェクトを渡せるようにする
- 外部から呼び出されて処理を実行している最中に、外部からその引数の内容を書き換えられてしまう危険性が潜んでいる
- メソッドの戻り値をMutableなオブジェクトで返す
- 引数の件と同様に意図しないタイミングで中身の書き換えが起こってしまう危険性がある(Immutableなオブジェクトであればそのような危険性はない)
- 仮にMutableなオブジェクトの場合でも、呼び出し側には読み取り専用のインターフェースの形でのみ戻り値を見せておけば、変更の危険性は減る
- メソッドの引数や戻り値にMapを使用する
- Mapはそのデータの意味するところを表現するのが難しく、わかりにくくなる。必ず詳細なコメントでそのデータの内容を説明しなければならなくなる
- メソッドがスローする例外を、そのメソッドの実装方式に寄ったものにする(例えばSQLExceptionをスローする等)
- 呼び出し側からすると内部の実装方式など知りたくない
- 実装方式が変わった場合に、スローする例外が増えたり変わったりして非常に面倒なことになる
補足
- 上記で書いた内容は、場合によりソースコードを読みにくくしたり、分かりにくくしたりということに直結しないこともあります。コンテキストに応じて項目の取捨選択や、読み替え等を行ってもらえればと思います。