149
101

Delete article

Deleted articles cannot be recovered.

Draft of this article would be also deleted.

Are you sure you want to delete this article?

More than 1 year has passed since last update.

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

Last updated at Posted at 2018-02-19

はじめに

当たり前だと思うのですが、意外と守られていないことがあります。その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など)では当てはまらない→その通りなので取り消しました。
  • 三項演算子は読みにくい→ケースバイケースなので、少し表現を弱めました。
  1. JavaScriptの場合: Logical Operators - JavaScript | MDN

  2. Javaコーディング規約 – Future Enterprise Coding Standards - フューチャーアーキテクト

  3. ヨーダ記法 - Wikipediaなど

  4. QBA: 論理式で boolean リテラル値を代入しているメソッド (QBA_QUESTIONABLE_BOOLEAN_ASSIGNMENT)

  5. 一応計測しましたが差は確認できませんでした。なので「理論上は」です。

  6. The Java® Virtual Machine Specificationから推測。

149
101
17

Register as a new user and use Qiita more conveniently

  1. You get articles that match your needs
  2. You can efficiently read back useful information
  3. You can use dark theme
What you can do with signing up
149
101

Delete article

Deleted articles cannot be recovered.

Draft of this article would be also deleted.

Are you sure you want to delete this article?