"".equals(stringVariable) は改悪だろう

「ひたすらプログラミング日記」経由で「次のJavaコードはどう改良できる？」を読んだ。

Javaで「if(stringVariable.equals(""))」と記述したとします。「stringVariableという文字列変数が何もない文字列であるときは」という意味のif文です。このif文は，もう少しうまい方法で記述可能です。それは次のどれでしょう？

1. if(stringVariable == "")
2. if(stringVariable == null)
3. if("".equals(stringVariable))

3のコードはたまに見かけるけど、こういうコードが広がると困る。 場合によるけど、これは一般的に「改良」じゃなくて「改悪」だろうね。

(1) ローカル変数の場合はコンパイラが null 初期化チェックをしてくれる

stringVariable がローカルで宣言した変数の場合は、初期化の有無をコンパイラがチェックしてくれる。 以下のコードは初期化されていない変数に対してメソッドを呼び出しているので、「変数 stringVariable は初期化されていない可能性があります」というコンパイルエラーになる。 なので、わざわざトリッキーな書き方をする必要はない。

String stringVariable;
if (stringVariable.equals("")) {   // この行でコンパイルエラー
  // なにか処理をやる
}

まぁ、このコンパイルエラーに対して、以下のように修正する人がいるんだけどね…。 せっかくの静的チェックが台無し。

String stringVariable = null;
if (stringVariable.equals("")) {
  // NullPointerException が発生する
}

宣言時に null で初期化するのも、 Java では良くない書き方。 まさか、「変数は null で初期化する」という規約を定めていて、「"".equals(stringVariable) と書くこと」という風習ができた…ことはないよね？

(2) 入力条件が明確じゃない 【重要】

stringVariable がローカル変数以外だとすると、外部からの入力値ということになる。 クラスやメソッドを作るときに重要なのは、インタフェースを明確にすること。 実装は簡単に変えられるけど、インタフェースはなかなか変えられない。 「stringVariable に null を許容する」というインタフェースにしたいんだろうけど、変更後のコードからはそれがハッキリとは読み取れない。 そもそも、以下のような条件分岐をすることがあるんだろうか。

if ("".equals(stringVariable)) {
  // stringVariableが空文字列の場合
} else {
  // stringVariableが「null」か「空以外の文字列」の場合
}

nullを曖昧にしておくと、以下のようなコードを平気で書いちゃったりする。 問題を先送りするほどにデバッグが困難になるよ。

if ("".equals(stringVariable)) {
  result = "default";
} else {
  // stringVariable が null の場合に NullPointerException となる
  result = stringVariable.toLowerCase();
}

(3) 可読性が落ちる

元のコードは、そのままで意図が「stringVariableが空文字列かどうか (if stringVariable equals "") 」という意図が伝わる。 でもリテラルを左に持ってくると「空文字列がstringVariableかどうか (if "" equals stringVariable) 」になってしまう。 主役が変数からではなくから文字列になっていて、可読性が落ちる。

Cでも「==」と「=」の書き間違えを防ぐために

if(CONST == variable)

というコードを見かけるけど、同じように可読性を落としている。

改良案1 (null条件を明記する)

null と空文字列を区別した上で、空文字列の場合だけに処理を書きたいのであれば、読み手にそのことが伝わるように書く。

if (stringVariable != null && stringVariable.equals("")) {
  // 空文字の場合の処理
}

改良案2 (明示的に初期化する)

元のコードとは意図が異なるけど、 null の場合に空文字列と同一視したいのであれば、先に初期化する。

if (stringVariable == null) {
  stringVariable = "";
}
if (stringVariable.equals("")) {
  // その後の処理
}

Ruby でよく見かける以下のようなコードも同じ意味。

# nil の場合は初期化.  str = str || "" という意味.
str ||= ""
array ||= []
hash ||= {}

改良案3 (nullを異常と明記する)

入力値として null を許容しないのであれば、最初に null チェックを入れておく。 問題は早めに見つけることが大切。

if (stringVariable == null) {
  throw new IllegalArgumentException("stringVariable が初期化されていません");
}
if (stringVariable.equals("")) {
  // その後の処理
}

まとめ

• トリッキーな書き方をせず、読み手に意図が伝わるように書こう
• ローカル変数の初期化の有無はコンパイラがチェックしてくれる
  • ローカル変数を宣言時に null で初期化するコードは何かが間違ってる
• 入力条件は、早めに確認しよう

stringVariable がメソッドの引数だろうが、何かのメソッドの戻り値だろうが、ちゃんと null について考慮することは重要。

• stringVariable が null の可能性はあるのか？それとも無いのか？
  • null の可能性があるなら、 null の場合はどのように扱うのか？
  • null の可能性がないなら、 null だった場合に例外を投げるか？それとも何もせずに NullPointerException を発生させるか？

ちゃんと考えておけば "".equals(stringVariable) なんて書く必要は全くないよ。

関連リンク

• Java6 では String クラスに isEmpty() メソッドが導入されているみたい。
• org.apache.commons.lang の StringUtils を使う。
  • (str == null || str.isEmpty()) 相当。nullと空文字を同一視したい場合はよさそう。
• 大抵の場合、どちらも入力されていないという扱いで十分ですので、nullと空白を同一の意味で扱ってもあまり問題ありません。Railsのblank?やStringUtils#isEmptyが短くて良いと思います。。
  • フォームからの入力値の場合はnullと空文字列が混在するので同一視するルールでよさそう。
• なんも考えずに"リテラル".equals(s)と書け。
  • 「「こちらのほうが美しい」という理由よりも、「こちらのほうが安全」という理由で書き方を選んだほうがいい。」にはとっても同意。「NullPointerExceptionへの遭遇は先延ばしにするべきだ」というのがスタンスの違いかなぁ。

追記

コンパイラはnullチェックをしてくれるんじゃなくて不定値のチェックをしてくれるだけという指摘を受けてnullチェック→初期化チェックに修正。