Edited at

Javaではif (isAdmin == true)というコードを書いてはいけない

More than 1 year has passed since last update.


はじめに

当たり前だと思うのですが、意外と守られていないことがあります。その1つの例として、条件分岐でのbooleanの扱いを挙げてみます。

タイトルには「Javaでは」と書きましたが、おそらくどの言語でも同じです。JavaScriptなど他の言語では当てはまらないので取り消します1


何がダメか

明確にコーディング規約で否定しているケースもありますが2、Javaでは以下のようなコードは書いてはいけません

if (isAdmin == true) {

// isAdminがtrueのとき
}

if (isAdmin == false) {
// isAdminがfalseのとき
}

理由を3つ書きます。


理由1: 冗長である

上記のコードは以下のように書くことができます。こちらの方がシンプルです。

if (isAdmin) {

// isAdminがtrueのとき
}

if (!isAdmin) {
// isAdminがfalseのとき
}

if〜else〜の場合は三項演算子で書くとシンプルになることもあります。ただし、三項演算子を乱用すると、可読性が落ちる場合があります。詳細は以下の記事を参照してください。

String userType;

if (isAdmin) {
userType = "管理者";
} else {
userType = "一般";
}

// 以下のように三項演算子で書くほうがシンプルになる
String userType = isAdmin ? "管理者" : "一般";

trueの場合は問題ないと思うのですが、falseの場合は!が目立ちにくいケースがあります。

if (!obj.getFoo().isBar()) {

// 処理
}

こういうときは、説明用変数を追加すると良いでしょう。

boolean bar = obj.getFoo().isBar();

if (!bar) {
// 処理
}


理由2: 代入と間違える可能性がある

Javaではif文に書くのはboolean型と決まっているので、if文に代入を書くと通常はコンパイルエラーになります。しかし、boolean型の場合、以下のように書いても、警告が出ずにコンパイルは通ってしまいます。

if (isAdmin = true) {

// isAdminがtrueのとき・・・のはずが常に実行される
}

if (isAdmin = false) {
// isAdminがfalseのとき・・・のはずが常に実行されない
}

// これはコンパイルエラーになる
if (isAdmin = 1) {
}

だからといって以下のようにヨーダ記法3で書くのは本末転倒です。

if (true == isAdmin) {

// isAdminがtrueのとき
}

if (false == isAdmin) {
// isAdminがfalseのとき
}

実際は、IDEやFindBugs/SpotBugs4で警告を出してくれるため、気づかないはずがないのですが、最初からif (isAdmin == true)という書き方をしなければ問題ありません。


理由3: (理論的には)遅くなる

以下の内容はb == trueのときのみで、b == false!bを使ったときは影響ありません。詳しくはコメントを参照してください。

これはほとんど余談ですが、理論的には無駄な処理が入ってしまいます5。例えば、以下のコードをコンパイルして、javap -cで逆アセンブルします。

public class Main {

public int foo(boolean b) {
if (b == true) {
return 1;
} else {
return 0;
}
}

public int bar(boolean b) {
if (b) {
return 1;
} else {
return 0;
}
}
}

すると、以下のような出力が得られます(関係ない出力は除いています)。

  public int foo(boolean);

Code:
0: iload_1
1: iconst_1
2: if_icmpne 7
5: iconst_1
6: ireturn
7: iconst_0
8: ireturn

public int bar(boolean);
Code:
0: iload_1
1: ifeq 6
4: iconst_1
5: ireturn
6: iconst_0
7: ireturn

これを読み解くと以下のようになります6。長々と書いていますが、要は命令が1つ無駄に多くなってしまいます。

なぜなら、if (b)は「bが真なら」を表しているのに対し、if (b == true)は、「bがtrueと比較した結果が真なら」と回りくどい表現になっているからです。


  • foo()


    • 0: ローカル変数1(引数b)をスタックに入れる

    • 1: 定数1(おそらくtrue)をスタックに入れる

    • 2: スタックの値(0:と1:)を比較し、違ってたら7:に移動

    • 5: 定数1(おそらく1)をスタックに入れる

    • 6: メソッドから値を戻す

    • 7: 定数0(おそらく0)をスタックに入れる

    • 8: メソッドから値を戻す



  • bar()


    • 0: ローカル変数1(引数b)をスタックに入れる

    • 1: スタックの値が0なら、6:に移動

    • 4: 定数1(おそらく1)をスタックに入れる

    • 5: メソッドから値を戻す

    • 6: 定数0(おそらく0)をスタックに入れる

    • 7: メソッドから値を戻す




おわりに

「これくらいどっちでもいいじゃないか」という人がいるかもしれませんが、こういうちょっとしたことの積み重ねが、将来大きな差となって表れます。少しずつでも良い習慣をつけていって欲しいです。


追記

反響があったようなので少しずつ補足していきます。



  • flag という変数名は悪い→その通りなので記述を追加しました。


    • それすら読んでもらえないようなので全部書き換えました。



  • 他の言語(JavaScriptなど)では当てはまらない→その通りなので取り消しました。

  • 三項演算子は読みにくい→ケースバイケースなので、少し表現を弱めました。