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 ;
}

Accepted Answer

私ならこう書きます。

``` Java
	public int getNum3(Integer num1, Integer num2) {
		if (num1 == null) return 0;
		if (num2 == null) return 0;
		return num2 - num1;
	}
```

Answer

いろんな意見があり面白いですね。

１０件目くらいの意見としては、リファクタリングがコードを易くする作業だとするならメソッドの定義に違和感をおぼえます。

引き算をするメソッドが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;

Answer

回答がもう出てますので勝手ながら比較部分のコードの指摘を。。。
int型は符号付き数値型です(2147483647 ～ -2147483648)
符号付き数値型の場合は、その最大の差をその型では表現できません。

例）2つのInteger型の最大の差は
| |  |
|:--|:--:|
|上限値|[Integer.MAX_VALUE](https://docs.oracle.com/javase/jp/8/docs/api/java/lang/Integer.html#MAX_VALUE)|2147483647|
|下限値|[Integer.MIN_VALUE](https://docs.oracle.com/javase/jp/8/docs/api/java/lang/Integer.html#MIN_VALUE)|-2147483648|

の時で値は4294967295になります。そのため比較部分と戻り値の型はlongになります。

```Java
    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

koko_uさんが既にご指摘されていますが、パラメタのnull判定をしてどちらかがnullだった場合の
```Java
return num3;
```
はちょっとどうかなと思います。return 0のほうがベターかなと。

例えばエラーを示す値が9だったとして、その場合、このような方針で似たような関数のリファクタリングをする際に、以下のようなコードにはしないと思います。

```Java
public int getNum3(Integer num1,Integer num2){ 
  int num3 =9; //注目！！
  if(num1==null&&num2==null){ 
    return num3; 
  } 
  num3 = num2-num1; 
  return num3 ; 
}
```

```Java
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するほうがベターだと思います。

```Java

//getNum3
//パラメタ２-パラメタ１の差を返却する
//パラメタのどちらかがnullの場合は0を返却する
public int getNum3(Integer num1,Integer num2){ 
  int num3;
  if(num1==null&&num2==null){ 
    return 0; 
  } 
  num3 = num2-num1; 
  return num3 ; 
}
```

あとは、これは投稿用に適当につけた名前だと思いますので本質的な指摘ではないのですが、可読性の観点からはもう少し変数名や関数名ににこだわると良いのではないでしょうか。

```Java

//getNumDifference
//パラメタ２-パラメタ１の差を返却する
//パラメタのどちらかがnullの場合は0を返却する
public int getNumDifference(Integer subtrahend,Integer minuend){ 
  int difference;
  if(subtrahend== null || minuend== null){ 
    return 0; 
  } 
  difference = minuend - subtrahend; 
  return difference; 
}
```

Answer

```java
	public int getNum3(Integer num1, Integer num2) {
		int num3 = 0;
		if (num1 != null && num2 != null) {
			num3 = num2 - num1;
		}
		return num3;
	}

```
単純にこれだとどうなるんだろう。

Answer

Java8のOptionalを使うとこんな感じ ```java public int getNum3(Integer num1,Integer num2){ Optional i1 = Optional.ofNullable(num1), i2 = Optional.ofNullable(num2); return i1.flatMap(x -> i2.map(y -> y - x)).orElse(0); } ``` 異常を伝えるためにもOptionalで返すべきですね。 ```java public Optional getNum3(Integer num1,Integer num2){ Optional i1 = Optional.ofNullable(num1), i2 = Optional.ofNullable(num2); return i1.flatMap(x -> i2.map(y -> y - x)); } ``` 参考→[JavaのOptionalのモナド的な使い方 - 複数のOptional](http://qiita.com/koher/items/6f4a8d8b3ad3142bf645#%E8%A4%87%E6%95%B0%E3%81%AEoptional)

Answer

では毛色の違ったこんなのはどうでしょうか。
```Java
public int getNum3(Integer num1, Integer num2) {
    return (getNum(num1) - getNum(num2));
}

public int getNum(Integer n) {
    return (n != null) n : 0;
}
```
--- 追記 ---
調べたらこんなんでいけそう
```Java
public int getNum3(Integer num1, Integer num2) {
    return (ObjectUtils.defaultIfNull(num1, 0) - ObjectUtils.defaultIfNull(num2, 0));
}
```
--- 追記 ---
ということで、他の方のを参考にしてください。(^_^;

Answer

私ならif文でなく↓の一行で戻り値返しします。

return (NULL != num1 && NULL != num2) ?  0 : num2 - num1;

Answer

Javaは書いたことないですが。。。

```java
public int getNum3(Integer num1, Integer num2){ 

		
        if(num1 == null || num2 == null) {
                return 0;
        }
		
        return num2 - num1;
	
}
```

```java
public int getNum3(Integer num1, Integer num2){ 

		
        return num1 == null || num2 == null ? 0 : num2 - num1;

	
}
```

Answer

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

関連した質問