Javaプログラマが知るべき9のこと

はじめに

ソースコードは設計であり、コードの記述は品質に直結するのは言うまでもない。ちなみに、プログラマにとって特に重要なのは保守性だ。コードは書いた直後から保守対象となるからだ。コードは要求文書の範囲で動けばいいと思っている人がいれば今すぐ、ソースコードをコピペして100klに増えるプラグインがいつの間にかインストールされる呪いをかけてあげよう。幸い、ここを読んでいる人にはそんな人はいないだろうと思うけれども。

ということで、コードの品質を下げる要因、すなわちシステム全体の品質を下げる要因となり、かつ使われやすいアンチパターンを挙げ、対策を検討していくことにする。対象は以下：

出力パラメータ
処理状態返却
意味のある配列
無意味な初期化
多すぎるtry-catch
暗黙の順序
コンパイラ警告の無視
過剰なコメント
e.printStackTrace()

出力パラメータ

メソッドの引数にオブジェクトを渡しメソッドの処理結果をそのオブジェクトに格納させる手法は、本当によく見ると思う。例えばこんな感じ：

HogeBean hoge = new HogeBean();
someFile.read(hoge);
out.println(hoge.getFuga());
out.println(hoge.getPiyo());

これの駄目なところは、someFile.readによってhogeオブジェクトの状態がどう変わるのかがreadメソッドを読まない限りわからないことだ。そういう箇所が一つであればまだマシだが、数箇所あったりifやwhileが関わったりするともう人の手には負えないコードになってしまう。「hogeの状態管理がsomeFile.readにまとまった」と思ったら大間違いだ。委託と丸投げくらいの違いがある。

そこはこう変えよう。

HogeBean hoge = new HogeBean();
hoge.setFuga(someFile.getFuga());
hoge.setPiyo(someFile.getPiyo());
out.println(hoge.getFuga());
out.println(hoge.getPiyo());

もう一歩進むとしたら、HogeBeanのコンストラクタにsomeFileを渡して、HogeBean内でfugaとpiyoをセットさせる方を選ぶ。が、とりあえず現時点では「hogeに対して外部からsetしてる時点で…」等という議論は避けることにしよう。一歩一歩いきましょうね！

処理状態返却

メソッドの返却値として正常終了したかどうかを返すパターンだ。

public int select(List<Hoge> list, String strDate) {
  try {
    List<Hoge> l = hogeDao.selectByDate(strDate);
    list.addAll(l);
    return 0;
  } catch (SQLException e) {
    log.error(e);
    return -1;
  }
}

これもよく見るだろう。intではなく独自のエラー処理用オブジェクトを使っていることも多い（今まで見た最も素敵なコードは"public Exception exec()"だ）。そして大抵、出力パラメータと仲がいい。というか、メソッドの処理状態を返すために貴重な返却値を使われてしまっているので、メソッドの処理結果はパラメータ上で渡す以外にない。

なんと言っても最大の欠点は、その異常処理状態がメソッド呼び出し側で無視される可能性が高いことだ。開発中のデバッグが難しくなるだけならまだマシだが、テストの網を潜り抜けてリリースされてしまう恐れがある。

ここは素直に例外処理を使うべきだ。ついでに出力パラメータも直してしまおう。

public List<Hoge> select(String strDate) {
  try {
    return hogeDao.selectByDate(strDate);
  } catch (SQLException e) {
    throw new RuntimeException(e);
  }
}

SQLExceptionをそのままthrowしてもいいが、ここではとりあえずRuntimeExceptionでラップした。よほど小規模のアプリケーションじゃない限り、SQLExceptionやIOExceptionはどこかの段階で何か別のExceptionにラップされるべきであり、十分な設計がされているとは思えないアプリケーションでも既に何らかの形でラップされているだろう。throws定義を伝播させるのが面倒すぎるからだ。どの段階でどんな例外として扱うかは考慮が必要となる。*1

メソッド呼び出し側が定義したlistにaddAllするかどうかは、呼び出し側に託すことにした。

繰り返すが、これはゴールでも模範解答でもなんでもない。一歩前より幾分マシにするだけの、小さなステップだ。

意味のある配列

配列の要素に意味を持たせたパターンだ。もちろん、Listでも同じ。「全てがListだ！」なんていうのはLisperにでも言わせておこう。

String[] destination= new String[3];
destination[0] = "920-8577";
destination[1] = "金沢市広坂1-1-1";
destination[2] = "金沢市役所";
return destination;

全てが暗黙の仕様となっている。

ここはクラスとして定義すべきだ。

public class Destination {
  public String zipcode;
  public String address;
  public String name;
}

// ...
Destination destination= new Destination();
destination.zipcode = "920-8577";
destination.address = "金沢市広坂1-1-1";
destination.name = "金沢市役所";
return destination;

情報隠蔽されていないクソクラスが増えたが、さっきよりマシだ（もっとも、setter/getterを使ったらこれよりマシになる、とは思わないが）。前者に比べて記述が増えてるが、その暗黙の仕様を書いたExcel方眼紙が要らなくなる。

大事なことなので繰り返すが、これは小さなステップでしかない。全ての問題が解決する例示なんて期待するほうが間違っている。

無意味な初期化

どこからの文化なのだろうか、変数の初期値としてリテラルやnewされたオブジェクトを代入しなければならないと思い込んでいる人は意外と多いようだ。

List<Hoge> hogeList = new ArrayList<Hoge>();
hogeList = HogeContainer.getList();

String fuga = "";
fuga = hoge.getFuga();

確かに、closeが必要なオブジェクトを扱う場面などで初期値の扱いが難しいことはある。が、それ以上に無駄に初期化しているコードは本当によく見る。

これは、1行にまとめよう。

List<Hoge> hogeList = HogeContainer.getList();

String fuga = hoge.getFuga();

まとめにくいと感じた場合は、変数のスコープが人の頭で管理できる限界を超えているかもしれない。どの変数がどこで必要になるのか、整理が必要だ。

似たようなパターンとして、スコープの最後でnullを代入しているパターンがある。オブジェクトのデストラクタのつもりだろうか。

多すぎるtry-catch

例外処理が、例外発生ごとに書かれているパターンだ。

public ErrorInfo exec(String param) {
  List<Model> models = null;
  BigDecimal dec = null;
  try {
    models = logic.readFromDB(param);
  } catch (SQLException e) {
    return new ErrorInfo("データベース接続でエラーが発生しました。");
  }

  try {
    dec = logic.sumUp(models);
  } catch (BussinessLogicException e) {
    return new ErrorInfo("集計処理でエラーが発生しました。");
  }

  try {
    out.write("合計：" + dec + "単位");
  } catch (IOException e) {
    return new ErrorInfo("出力部分でエラーが発生しました。");
  }

  return null;
}

通常の処理と様々なレベル（プログラム由来やデータ由来、設定由来など）のエラー処理とが混在し、ここで何をしたいのか不明確だ。また、それぞれの処理がtry-catchブロックで分かれるため、ブロック間で情報を伝達させるためのローカル変数を冒頭で宣言しなければならず、必然的に無意味な初期値を与えざるを得ない。そもそも、各メソッドで非チェック例外（NullPointerExceptionなど）が起こったとして、適切にエラー処理がされるのだろうか？

処理状態返却パターンでの構造を例外処理構造にそのまま適用したのだろう。黄金のアンチパターンとも呼べるようなコードが仕上がった。

try-catchブロックは一つにまとめてしまおう。

public ErrorInfo exec(String param) {
  try {
    List<Model> models = logic.readFromDB(param);
    BigDecimal dec = logic.sumUp(models);
    out.write("合計：" + dec + "単位");
  } catch (SQLException e) {
    return new ErrorInfo("データベース接続でエラーが発生しました。");
  } catch (BussinessLogicException e) {
    return new ErrorInfo("集計処理でエラーが発生しました。");
  } catch (IOException e) {
    return new ErrorInfo("出力部分でエラーが発生しました。");
  }
  return null;
}

ただし、投げられるExceptionクラスが混在している場合はcatchで分離できないため、少し厄介になる。上記コードでは、最低限としてスタックトレース情報をErrorInfoに管理させる必要があるだろう。

また、それぞれのcatchブロックで何か特別な処理をしている場合、例えばBussinessLogicExceptionをcatchした中でreturnせずにdecの初期値を設定している場合は、try-catchをまとめることはできない。例外処理構造に本来の（「例外」ではない）処理を混在させてしまっているせいだ。try-catchをネストさせるか、もしくはメソッド内部に収まらない変更が必要だろう。

例外処理について考えるための、いいきっかけになるかもしれない。

暗黙の順序

例えば、インスタンス変数の初期値を設定するメソッドと、それを使って何かをするメソッドが分かれているようなパターンだ。

public class Hoge {
  private Props props;

  /**
   * インスタンスを初期化する
   */
  public void init() {
    this.props = new Props();
  }

  /**
   * プロパティから設定を読み込む
   */
  public void setProps(Properties p) {
    props.setCompanyName(p.get("company.name"));
  }

  /**
   * ようこそメッセージを返す
   */
  public String welcomeMessage() {
    return props.getCompanyName() + "へようこそ";
  }
}

知らずに使うと、welcomeMessageを使ってぬるぽが起こり、初めてinitする必要があることを知るだろう。initしてwelcomeMessageを使ってみたが、得られるのは「へようこそ」「へようこそ」「へようこそ」。「へ」って何じゃ！！進捗は1日遅れです。

上のコードのような小さいクラスだとsetPropsに気付くだろうが、現場にコミットされているクラスはそんなもんじゃなかろう。2000行の中から探し出さなければならないとしたら、人智を超越した何かへの挑戦だ。そして、ベタ書き派は喜んで言う。「ほら、処理を分けると読み難くなるだろっ？」

インスタンスの前提条件があるとしたら、インスタンス変数の初期値としたり、コンストラクタにまとめよう。

public class Hoge {
  private Props props = new Props();

  /**
   * プロパティから値を読み込む
   */
  public Hoge(Properties p) {
    props.setCompanyName(p.get("company.name"));
  }

  /**
   * ようこそメッセージを返す
   */
  public String welcomeMessage() {
    return props.getCompanyName() + "へようこそ";
  }
}

これで、このインスタンスは必ずpropsを保持していることが保証できた。インスタンスが扱える状態であれば常に、それっぽいwelcomeMessageを出せるだろう。

もう一歩進めるとしたら、propsをfinalとするのもいい。さらにもう一歩であれば、Propsクラスが保持しているだろうCampanyNameもfinalとしたい（そのためにはsetterアクセスをちょっと変える必要はある）。そう、不変クラスとかイミュータブルと呼ばれるヤツだ。インスタンスの状態遷移を排除すると、順序関係を逆戻りさせるような操作がないこともわかるようになる。*2

DIコンテナでセッターインジェクションを使ってるとしたら、…まぁそれもいいだろう。その場合はメソッドの先頭で前提条件に合わない場合に例外投げたり（メッセージは分かりやすくしておこう）、最低限Javadocに何か書いてくれると助かるかもしれない。もっとも、DIコンテナは詳しくないので、最近は解決できる何かがあるのかもしれないが。

コンパイラ警告の無視

EclipseでのWarning（黄色い「！」）のことを、ここではコンパイラ警告と呼ぶことにしよう。不要なimport、未使用のprivateフィールドやメソッド、未使用の引数やローカル変数、型パラメータの省略、インスタンスを通じたstaticアクセス、非推奨APIの使用…、現場のコードでよく見るのはこれくらいだろう。

import java.util.ArrayList;
import java.util.Date;
import java.util.LinkedList;
import java.util.List;

import org.junit.Test;

public class ALotOfWarnings {
  private List alist;
  private List anotherlist;
  private String hoge = "hoge";

  public ALotOfWarnings() {
    alist = new ArrayList();
    alist.add("hoge");
    alist.add(Date.UTC(2011, 2, 4, 11, 22, 33));
  }

  public Date Foo() {
    return (Date) alist.get(1);
  }

  private void makeAnotherlistSomething() {
    anotherlist = new ArrayList();
  }
}

このコードが正常に動くだろうか？JVMの立場からは、正常に動くだろう。それ以上は何ともいえない。

直せるところから直してみよう。

import java.text.ParseException;
import java.text.SimpleDateFormat;
import java.util.ArrayList;
import java.util.Date;
import java.util.List;

public class ALotOfWarnings {
  private List alist;
  private String hoge = "hoge";
  private SimpleDateFormat df = new SimpleDateFormat("yyyy-MM-dd hh:mm:ss");

  public ALotOfWarnings() {
    alist = new ArrayList();
    alist.add(hoge);
    try {
      alist.add(df.parse("2011-03-04 11:22:33"));
    } catch (ParseException e) {
      throw new RuntimeException(e);
    }
  }

  public Date Foo() {
    return (Date) alist.get(1);
  }
}

これでも、alistに総称型パラメータを指定していないとう警告は残る。上記「意味のある配列」パターンが潜んでいることが明確になった。これ以上は、この短いコードなら答えを出すのは簡単だが、実際の製品中のコードで短時間で答えを出せるとは限らないだろう。

コンパイラ警告はアンチパターン集だ。それ自体大きな問題にはならないことも多いが、設計パターンに関して重要なヒントを与える。活用しない手はない。

過剰なコメント

Subversion等でバージョン管理がなされている環境であれば、改変履歴コメントや改変元コードのコメントアウトは害悪でしかない。diffが取りづらくなるからだ。他にも単純なsetter/getterのJavadoc等は管理すべき資産を無駄に増やすので、付けるべきではない。

/**
 * hogeをセットする。<br/>
 * #456 2011-02-04 katzchang 0以下の場合は0をセットするように変更
 * @param hoge
 */
public void setHoge(int hoge) {
//del start #456 2011-02-04 katzchang
//  this.hoge = hoge;
//del end #456 2011-02-04 katzchang
//add start #456 2011-02-04 katzchang
  if(hoge > 0) {
    this.hoge = hoge;
  } else {
    this.hoge = 0;
  }
//add end #456 2011-02-04 katzchang
}

/**
 * hogeを返却する。
 * @return hoge
 */
public int getHoge() {
  return hoge;
}

ここはシンプルにいこう：

/**
 * hogeをセットする。<br/>
 * ただし、0以下の場合には0をセットする。
 */
public void setHoge(int hoge) {
  if(hoge > 0) {
    this.hoge = hoge;
  } else {
    this.hoge = 0;
  }
}

public int getHoge() {
  return hoge;
}

適切に管理されたJavadocコメントは本当に役に立つ。しかしコピペ駆動開発現場では、コピペ元の@paramが残っているなどのワナが残されることが多い。機械的にチェックできるプラグインがあるはず（たぶん。なければ誰か作るべき）なので、Javadocを残さなければならないとしたら、導入を検討しよう。*3

e.printStackTrace()

catch句で、なぜかprintStackTrace()が呼ばれているパターンだ。*4

try {
  //...
} catch (SQLException e) {
  e.printStackTrace();
  throw new HogeException("データベース接続でエラーが発生しました");
}

さて、ここまで「うんうん」と頷きながら読み進めた人であれば、これの何がダメでどうすべきかは知っているだろう。解決する道筋はいくつかあるが、そう難しい話はない。問題はここから。

職場でこういったコードを見たことがある人は、同僚に対して潜在する問題点を説明し、改善案を提示するまでのストーリーを考えてほしい。納得してもらい、円滑にコードを修正する。これができなければ、今までの解決策も含め、すべてがただのこだわりプログラマのウンコ主張に終わってしまう。本当に難しい問題はここにある。

まとめ

小さな一歩を繰り返してクソコードをマシにしていくことで、ようやくコードは設計書と呼べるレベルに近づいていくだろう。クソコードを放置することはコード自体の管理の手間が増えるだけではなく別文書の複雑な設計書の管理も必要とさせ、保守コストに二重に跳ね返る。デバッグで終わらせる人生なんて馬鹿げていると思ったら、一歩ずつでいい、コードを整理していこう。完璧である必要なんてない。

そして、ほとんどのことはたぶんEffective Javaに書いてある（読んだことないけど）。それでなくても幾度となく繰り返されている議論であるはずだが、やはり届いていない現場は多い。さてこの状況、私たちに何ができるだろうか？

Effective Java 第2版 (The Java Series)

作者: Joshua Bloch,柴田芳樹
出版社/メーカー: ピアソンエデュケーション
発売日: 2008/11/27
メディア: 単行本（ソフトカバー）
購入: 77人クリック: 936回
この商品を含むブログ (264件) を見る

おわりに

ブクマで「id:kyon_mm 多分この辺はCleanCodeのほうが近い内容かな。」とあったけど、書評を見る限りそう思います（さっき注文した）。著者アンクル・ボブが、プログラマが知るべき97のことに寄稿した「08：ボーイスカウトルール」で書いていることに似ている…ような気もしないでもないというとアレなんかな。

ということで、あわせて読みたい：