if文のリファクタリング

Question

現在Javaでif文のコードを書いています。

if文のコードの良い書き方(リファクタリング)について教えて頂きたいです。

次のコードはどのように書くのがベターでしょうか？

Integer型の変数num1，num2があり、どちらもnullでない場合、差を計算し、その結果を返すメソッドです。

私は以下のように書きましたが、もっといい書き方があれば教えて頂きたいです。
if文のreturnは早く返した方がいいとリファクタリングの本で見た気がするのでその観点で実装しています。

public int getNum3(Integer num1,Integer num2){
int num3 =0;
if(num1==null&&num2==null){
return num3;
}
num3 = num2-num1;
return num3 ;
}

Answer 1

私ならこう書きます。

	public int getNum3(Integer num1, Integer num2) {
		if (num1 == null) return 0;
		if (num2 == null) return 0;
		return num2 - num1;
	}

Comments

[mike2mike4]

可読性が優れているんじゃないでしょうか

[k499778]

みなさん回答ありがとうございました。
ご親切なみなさまのアドバイスのおかげで疑問が解消されたと同時に多くの新しい知識や書き方も知ることができました。
BAは一番わかりやすかったという点でこちらの回答を選ばせていただきました。
nullのわずらわしさを払拭してくれるOptionalも興味があるので今後触れていきたいと思います。

[kometaroimo]

美しいコードですねぇ

Answer 2

回答がもう出てますので勝手ながら比較部分のコードの指摘を。。。
int型は符号付き数値型です(2147483647 ～ -2147483648)
符号付き数値型の場合は、その最大の差をその型では表現できません。

例）2つのInteger型の最大の差は

上限値	Integer.MAX_VALUE
下限値	Integer.MIN_VALUE

の時で値は4294967295になります。そのため比較部分と戻り値の型はlongになります。

    public long getNum3(Integer num1, Integer num2) {
        if (num1 == null || num2 == null) {
            return 0;
        }
        return (long)num2 - num1;
    }

getNum3(Integer.MIN_VALUE, Integer.MAX_VALUE); をこの値を引数に渡して見るとと分かりやすいかもーです。

Answer 3

Java8のOptionalを使うとこんな感じ

public int getNum3(Integer num1,Integer num2){
    Optional<Integer> i1 = Optional.ofNullable(num1),
                      i2 = Optional.ofNullable(num2);
    return i1.flatMap(x -> i2.map(y -> y - x)).orElse(0);
}

異常を伝えるためにもOptionalで返すべきですね。

public Optional<Integer> getNum3(Integer num1,Integer num2){
    Optional<Integer> i1 = Optional.ofNullable(num1),
                      i2 = Optional.ofNullable(num2);
    return i1.flatMap(x -> i2.map(y -> y - x));
}

参考→JavaのOptionalのモナド的な使い方 - 複数のOptional

Answer 4

num1==null&&num2==null ではなく num1==null || num2==nullですよね？書き方は普通だと思います。
私なら、先にリターンする方は return num3;ではなく、直接return 0;と書きますが。

Comments

[koko_u]

ちなみに、3項演算子はめったに使いません。

> if文のreturnは早く返した方がいいとリファクタリングの本で見た気がするのでその観点で実装しています。

これは、メソッドの実装として

1. 事前チェック(例えば、引数に問題がないかを確かめる)。問題があれば処理をしない。
2. よさそうなら本来やりたかった処理をする

という意図が明確になるようにコードを書くべき、ということです。

今回、引数が null であるケースと null でないケースが「並列」ではないので、3項演算子で return 1文でまとめていると、コードレビューで私はNGを出す可能性が高いです。

Answer 5

私ならif文でなく↓の一行で戻り値返しします。

return (NULL != num1 && NULL != num2) ? 0 : num2 - num1;

Comments

[swordone]

Javaで大文字のNULLって使えたっけ？

[Zuishin]

null ですね。それと && ではなく || が正しいです。

[root_jp]

使えないですね。

[HogeAnimalLover]

ああ失礼
(null != num1 && null != num2) ? num2 - num1 : 0;
ですね。

Answer 6

いろんな意見があり面白いですね。

１０件目くらいの意見としては、リファクタリングがコードを易くする作業だとするならメソッドの定義に違和感をおぼえます。

引き算をするメソッドがgetNum3と言う点のツッコミをほっといても、引き算をするメソッドでNullを許容している点はおかしいと思います。さらに、その時の戻り値が０これって以下のケースですべて戻り値が０になると言うことです。

getNum3(1000, 1000) == 0;
getNum3(0, 0) == 0;
getNum3(Null, 84) == 0;
getNum3(-553, Null) == 0;

どういう要件か解りませんがこれが正しい動作である気がしません。
そのまま例外を投げるか、Null伝播が適当だと思われます。

getNum3(1000, 1000) == 0;
getNum3(0, 0) == 0;
getNum3(Null, 84) == Null;
getNum3(-553, Null) == Null;

Comments

[swordone]

そういう意味では私の方法ではOptionalで返したい。

[iwamoto_takaaki]

Optionalで返すもの優れた選択肢だと思います。それも入れておくべきでした。（例外/Optional/Nullのどれを選ぶのかは、場合によって変わってくるとおもいます。）
異常な入力に対して、正常な値を返すのは良くないという点は同意してもらえるとおもいます。

[k499778]

回答ありがとうございます。
それでいうと要件としては以下になります。

1.web apiを使ってデータが送られてくる。
2.データがないときは配列に項目がないためNULLとなる
3.num2が全体の数値、それからnum1を引いたものがnum3

そのため実際にはこのように書きました。

public int getNum3(Integer num1, Integer num2) {
if (num2 == null) return 0;
if (num1 == null) return num2;
return num2 - num1;
}

[iwamoto_takaaki]

なるほど、APIの仕様だろうけど、入力はNullになるところが少し違和感があります。空の配列が帰ってきていたら入力は０になるのに・・・（XMLやJSONなどだったら仕方がないかも・・・）

逆に意味論から言うと、こんなコードが意図がつかみやすいと思います。

public int culcRemainderCount(int index, int all){
if(index == null) index = 0;
if(all == null) all = 0;
return all - index;
}

メソッド名、変数名などは適当ですが、変数がnullの時は０とみなしているという意図がはっきりしているのが判るとおもいます。
ガード節を使う方が効率的になりますが、実行速度を上げるのはリファクタリングの目的にはならないので、読みやすさを優先させるのがよいと思います。

[k499778]

回答ありがとうございます。
大変わかりやすく自分にはない発想なのでまた武器が１つ増えました。

そうですね。こちらのコードを参考にさせて頂きます。

Answer 7

koko_uさんが既にご指摘されていますが、パラメタのnull判定をしてどちらかがnullだった場合の

return num3;

はちょっとどうかなと思います。return 0のほうがベターかなと。

例えばエラーを示す値が9だったとして、その場合、このような方針で似たような関数のリファクタリングをする際に、以下のようなコードにはしないと思います。

public int getNum3(Integer num1,Integer num2){ 
  int num3 =9; //注目！！
  if(num1==null&&num2==null){ 
    return num3; 
  } 
  num3 = num2-num1; 
  return num3 ; 
}

public int getNum3(Integer num1,Integer num2){ 
  int num3 =0;
  if(num1==null&&num2==null){ 
    num3 = 9; //注目！！
    return num3; 
  } 
  num3 = num2-num1; 
  return num3 ; 
}

可読性が落ちますし、後者は処理も無駄に増えてしまっています。
エラーや、何も処理しなかったことを示す値が明確に決まっている場合は、その値を直接returnするほうがベターだと思います。

//getNum3
//パラメタ２-パラメタ１の差を返却する
//パラメタのどちらかがnullの場合は0を返却する
public int getNum3(Integer num1,Integer num2){ 
  int num3;
  if(num1==null&&num2==null){ 
    return 0; 
  } 
  num3 = num2-num1; 
  return num3 ; 
}

あとは、これは投稿用に適当につけた名前だと思いますので本質的な指摘ではないのですが、可読性の観点からはもう少し変数名や関数名ににこだわると良いのではないでしょうか。

//getNumDifference
//パラメタ２-パラメタ１の差を返却する
//パラメタのどちらかがnullの場合は0を返却する
public int getNumDifference(Integer subtrahend,Integer minuend){ 
  int difference;
  if(subtrahend== null || minuend== null){ 
    return 0; 
  } 
  difference = minuend - subtrahend; 
  return difference; 
}

Comments

[fuzzball]

minuendとsubtrahendが逆では？

[akabee]

リファクタリングしていただきありがとうございます。その通りですね。
自戒もこめて修正しておきます。

Answer 8

	public int getNum3(Integer num1, Integer num2) {
		int num3 = 0;
		if (num1 != null && num2 != null) {
			num3 = num2 - num1;
		}
		return num3;
	}

単純にこれだとどうなるんだろう。

Answer 9

では毛色の違ったこんなのはどうでしょうか。

public int getNum3(Integer num1, Integer num2) {
    return (getNum(num1) - getNum(num2));
}

public int getNum(Integer n) {
    return (n != null) n : 0;
}

--- 追記 ---
調べたらこんなんでいけそう

public int getNum3(Integer num1, Integer num2) {
    return (ObjectUtils.defaultIfNull(num1, 0) - ObjectUtils.defaultIfNull(num2, 0));
}

--- 追記 ---
ということで、他の方のを参考にしてください。(^_^;

Comments

[swordone]

これだと片方だけnullの時に0を返せない(num1がnull,num2が3だとしたら、0-3で‐3になる)

[takasima20]

おっといけねー。指摘ありがとうございます。

[takasima20]

akabeeさんが書かれてるように、0が正常な計算結果なのかイリーガルな処理結果なのか判別できないってのが難しいとこですよねえ。

[takasima20]

自分的にはTakeOneさんのが分かりやすくていいかな～

Answer 10

Javaは書いたことないですが。。。

public int getNum3(Integer num1, Integer num2){ 

		
        if(num1 == null || num2 == null) {
                return 0;
        }
		
        return num2 - num1;
	
}

public int getNum3(Integer num1, Integer num2){ 

		
        return num1 == null || num2 == null ? 0 : num2 - num1;

	
}

Comments

[koko_u]

=== は JavaScript なんじゃよ...

[退会済みユーザー]

なんだって〜〜〜。。。

[退会済みユーザー]

修正しました笑

[koko_u]

よか。よか。

[Zuishin]

それとやはり && はバグで koko_u さんの指摘通り || にしないといけません。

[退会済みユーザー]

なおしましたん！