質問をすることでしか得られない、回答やアドバイスがある。

15分調べてもわからないことは、質問しよう!

ただいまの
回答率

88.92%

if文のリファクタリング

解決済

回答 10

投稿

  • 評価
  • クリップ 0
  • VIEW 2,356

k499778

score 542

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

  • 気になる質問をクリップする

    クリップした質問は、後からいつでもマイページで確認できます。

    またクリップした質問に回答があった際、通知やメールを受け取ることができます。

    クリップを取り消します

  • 良い質問の評価を上げる

    以下のような質問は評価を上げましょう

    • 質問内容が明確
    • 自分も答えを知りたい
    • 質問者以外のユーザにも役立つ

    評価が高い質問は、TOPページの「注目」タブのフィードに表示されやすくなります。

    質問の評価を上げたことを取り消します

  • 評価を下げられる数の上限に達しました

    評価を下げることができません

    • 1日5回まで評価を下げられます
    • 1日に1ユーザに対して2回まで評価を下げられます

    質問の評価を下げる

    teratailでは下記のような質問を「具体的に困っていることがない質問」、「サイトポリシーに違反する質問」と定義し、推奨していません。

    • プログラミングに関係のない質問
    • やってほしいことだけを記載した丸投げの質問
    • 問題・課題が含まれていない質問
    • 意図的に内容が抹消された質問
    • 過去に投稿した質問と同じ内容の質問
    • 広告と受け取られるような投稿

    評価が下がると、TOPページの「アクティブ」「注目」タブのフィードに表示されにくくなります。

    質問の評価を下げたことを取り消します

    この機能は開放されていません

    評価を下げる条件を満たしてません

    評価を下げる理由を選択してください

    詳細な説明はこちら

    上記に当てはまらず、質問内容が明確になっていない質問には「情報の追加・修正依頼」機能からコメントをしてください。

    質問の評価を下げる機能の利用条件

    この機能を利用するためには、以下の事項を行う必要があります。

回答 10

checkベストアンサー

+10

私ならこう書きます。

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

投稿

  • 回答の評価を上げる

    以下のような回答は評価を上げましょう

    • 正しい回答
    • わかりやすい回答
    • ためになる回答

    評価が高い回答ほどページの上位に表示されます。

  • 回答の評価を下げる

    下記のような回答は推奨されていません。

    • 間違っている回答
    • 質問の回答になっていない投稿
    • スパムや攻撃的な表現を用いた投稿

    評価を下げる際はその理由を明確に伝え、適切な回答に修正してもらいましょう。

  • 2017/05/17 23:57

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

    キャンセル

  • 2017/05/18 00:10

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

    キャンセル

  • 2017/05/18 13:29

    美しいコードですねぇ

    キャンセル

+4

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

投稿

  • 回答の評価を上げる

    以下のような回答は評価を上げましょう

    • 正しい回答
    • わかりやすい回答
    • ためになる回答

    評価が高い回答ほどページの上位に表示されます。

  • 回答の評価を下げる

    下記のような回答は推奨されていません。

    • 間違っている回答
    • 質問の回答になっていない投稿
    • スパムや攻撃的な表現を用いた投稿

    評価を下げる際はその理由を明確に伝え、適切な回答に修正してもらいましょう。

  • 2017/05/17 23:10

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

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

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

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

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

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

    キャンセル

+4

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

投稿

編集

  • 回答の評価を上げる

    以下のような回答は評価を上げましょう

    • 正しい回答
    • わかりやすい回答
    • ためになる回答

    評価が高い回答ほどページの上位に表示されます。

  • 回答の評価を下げる

    下記のような回答は推奨されていません。

    • 間違っている回答
    • 質問の回答になっていない投稿
    • スパムや攻撃的な表現を用いた投稿

    評価を下げる際はその理由を明確に伝え、適切な回答に修正してもらいましょう。

+4

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

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

上限値 Integer.MAX_VALUE 2147483647
下限値 Integer.MIN_VALUE -2147483648

の時で値は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); をこの値を引数に渡して見るとと分かりやすいかもーです。

投稿

編集

  • 回答の評価を上げる

    以下のような回答は評価を上げましょう

    • 正しい回答
    • わかりやすい回答
    • ためになる回答

    評価が高い回答ほどページの上位に表示されます。

  • 回答の評価を下げる

    下記のような回答は推奨されていません。

    • 間違っている回答
    • 質問の回答になっていない投稿
    • スパムや攻撃的な表現を用いた投稿

    評価を下げる際はその理由を明確に伝え、適切な回答に修正してもらいましょう。

+2

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

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

投稿

編集

  • 回答の評価を上げる

    以下のような回答は評価を上げましょう

    • 正しい回答
    • わかりやすい回答
    • ためになる回答

    評価が高い回答ほどページの上位に表示されます。

  • 回答の評価を下げる

    下記のような回答は推奨されていません。

    • 間違っている回答
    • 質問の回答になっていない投稿
    • スパムや攻撃的な表現を用いた投稿

    評価を下げる際はその理由を明確に伝え、適切な回答に修正してもらいましょう。

  • 2017/05/17 22:54

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

    キャンセル

  • 2017/05/17 22:57

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

    キャンセル

  • 2017/05/17 22:58

    使えないですね。

    キャンセル

  • 2017/05/17 23:03 編集

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

    キャンセル

+1

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;


}

投稿

編集

  • 回答の評価を上げる

    以下のような回答は評価を上げましょう

    • 正しい回答
    • わかりやすい回答
    • ためになる回答

    評価が高い回答ほどページの上位に表示されます。

  • 回答の評価を下げる

    下記のような回答は推奨されていません。

    • 間違っている回答
    • 質問の回答になっていない投稿
    • スパムや攻撃的な表現を用いた投稿

    評価を下げる際はその理由を明確に伝え、適切な回答に修正してもらいましょう。

  • 2017/05/17 22:40

    よか。よか。

    キャンセル

  • 2017/05/17 22:55

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

    キャンセル

  • 2017/05/17 23:08

    なおしましたん!

    キャンセル

+1

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

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


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

投稿

編集

  • 回答の評価を上げる

    以下のような回答は評価を上げましょう

    • 正しい回答
    • わかりやすい回答
    • ためになる回答

    評価が高い回答ほどページの上位に表示されます。

  • 回答の評価を下げる

    下記のような回答は推奨されていません。

    • 間違っている回答
    • 質問の回答になっていない投稿
    • スパムや攻撃的な表現を用いた投稿

    評価を下げる際はその理由を明確に伝え、適切な回答に修正してもらいましょう。

  • 2017/05/17 23:14

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

    キャンセル

  • 2017/05/17 23:16

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

    キャンセル

  • 2017/05/17 23:33

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

    キャンセル

  • 2017/05/17 23:42

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

    キャンセル

+1

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


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

投稿

  • 回答の評価を上げる

    以下のような回答は評価を上げましょう

    • 正しい回答
    • わかりやすい回答
    • ためになる回答

    評価が高い回答ほどページの上位に表示されます。

  • 回答の評価を下げる

    下記のような回答は推奨されていません。

    • 間違っている回答
    • 質問の回答になっていない投稿
    • スパムや攻撃的な表現を用いた投稿

    評価を下げる際はその理由を明確に伝え、適切な回答に修正してもらいましょう。

+1

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
//パラメタ2-パラメタ1の差を返却する
//パラメタのどちらかがnullの場合は0を返却する
public int getNum3(Integer num1,Integer num2){ 
  int num3;
  if(num1==null&&num2==null){ 
    return 0; 
  } 
  num3 = num2-num1; 
  return num3 ; 
}

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

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

投稿

編集

  • 回答の評価を上げる

    以下のような回答は評価を上げましょう

    • 正しい回答
    • わかりやすい回答
    • ためになる回答

    評価が高い回答ほどページの上位に表示されます。

  • 回答の評価を下げる

    下記のような回答は推奨されていません。

    • 間違っている回答
    • 質問の回答になっていない投稿
    • スパムや攻撃的な表現を用いた投稿

    評価を下げる際はその理由を明確に伝え、適切な回答に修正してもらいましょう。

  • 2017/05/18 15:00

    minuendとsubtrahendが逆では?

    キャンセル

  • 2017/05/18 15:10

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

    キャンセル

+1

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

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

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

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;

投稿

  • 回答の評価を上げる

    以下のような回答は評価を上げましょう

    • 正しい回答
    • わかりやすい回答
    • ためになる回答

    評価が高い回答ほどページの上位に表示されます。

  • 回答の評価を下げる

    下記のような回答は推奨されていません。

    • 間違っている回答
    • 質問の回答になっていない投稿
    • スパムや攻撃的な表現を用いた投稿

    評価を下げる際はその理由を明確に伝え、適切な回答に修正してもらいましょう。

  • 2017/05/18 17:57

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

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

    キャンセル

  • 2017/05/18 19:30 編集

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

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

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

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

    キャンセル

  • 2017/05/18 21:04

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

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

    キャンセル

15分調べてもわからないことは、teratailで質問しよう!

  • ただいまの回答率 88.92%
  • 質問をまとめることで、思考を整理して素早く解決
  • テンプレート機能で、簡単に質問をまとめられる

関連した質問

同じタグがついた質問を見る