Hello.java の変更点

更新
追加された行はこの色です。
削除された行はこの色です。
プログラミング/作法/Hello.java へ行く。
プログラミング/作法/Hello.java の差分を削除
[[公開メモ]]

* 概要 [#f7d6579a]

Mitsuyuki.Shiiba さんという方が、「僕の好きなコードの書き方」というタイトルで
ブログを書いてらっしゃって、その中でちょっとあれなコードを公開でリファクタリングしていたのですが、
例として挙げられたコードが秀逸だったので自分でもリファクタリングにトライしてみました((本当はテストのない状態でのコード改善をリファクタリングと呼んではいけません。よい子の皆さんはまねしないで下さい。))
。

＃ というのが私のスタンスだったのですが、Mitsuyuki.Shiiba さんご自身は、~
＃ “想像してたシチュエーションは「プロトタイピング」”~
＃ とのことで、両者のスタンスはかなり違ったものになりました。~
＃ 結果的にはその雰囲気の違いなんかも改善過程に良く現れていて面白いことになったのではと思います。~

オリジナルのコードが Gist 上に https://gist.github.com/bufferings/9414995751687177e882 にあったのですが、特定のリビジョンから Fork する方法が解らなくて、私の方でコピペで新しい Gist を作ることになってしまいました。たぶん間違った方法なんだと思います。どうもすみません。

ここからも解るように、私は Gist も、実は Java もあまりよく分かっていません。普段は Ruby とか JavaScript とか C# とか良く書いてます。Java のコンパイル環境もなしでやるので、タイプミスについてもご容赦下さい。

** 本家の記事更新状況 [#g84a59f3]

- Mitsuyuki.Shiiba トップ http://bufferings.hatenablog.com/
- 僕の好きなコードの書き方 http://bufferings.hatenablog.com/entry/2015/05/08/091328
- 続：僕の好きなコードの書き方 http://bufferings.hatenablog.com/entry/2015/05/08/091328
- 続々：僕の好きなコードの書き方 http://bufferings.hatenablog.com/entry/2015/05/13/090614
- 続々々：僕の好きなコードの書き方 http://bufferings.hatenablog.com/entry/2015/05/25/090536
- 続々々々：僕の好きなコードの書き方 http://bufferings.hatenablog.com/entry/2015/05/27/090941
- 続々々々々：僕の好きなコードの書き方 http://bufferings.hatenablog.com/entry/2015/05/28/090652
- 続々々々々々：僕の好きなコードの書き方 http://bufferings.hatenablog.com/entry/2015/06/02/225224
- まとめ：僕の好きなコードの書き方 http://bufferings.hatenablog.com/entry/2015/06/03/082323

完結しました。~
とても楽しめました。ありがとうございました！

** 他にもやってらっしゃる方がいました [#af162388]

:@irof さん| https://github.com/irof/sandbox/tree/master/hello-bufferings

** このページの目次 [#z4a1a8a5]

#contents

* オリジナルのソース [#j3fe1530]

https://gist.github.com/osamutake/eef52076810a996831c6/df8fbfa8c1b2976feeebb8c2c70fd62fa3f87a2a

#gist(osamutake/eef52076810a996831c6/df8fbfa8c1b2976feeebb8c2c70fd62fa3f87a2a);

** 方針 [#efdc29c6]

関数名や変数名が意味を持っていないため、全体として何をするためのコードかまったく解りません。

仕方がないので上から順に読みながら改善していくことにします。

** ところで [#qb68ba52]

ここは一旦全部書いてから戻ってきて書いているのですが、

上記のコードはコード改善の例として使うには大変良い物だと思います。

こういった話に興味があって、お時間のある方は、是非ご自分でもやってみると、とても楽しめると思います。

改善の方向は必ずしも一本槍にならないので、
個性溢れるコードがいろいろできて、後から他人のコードと比較すると様々な発見があるんじゃないかと思います。

私もいろんなコードを是非見てみたいです。

~
~
~
~
~
~
~
~
~
~
~
~
~
~
~

と、前置きは以上にして、いよいよ進めていきます。

* 6 行目, 7 行目の if の連鎖によりコードのインデントが深くなってしまっている。 [#yc870704]

このコード、こんな構造になっています。

 LANG:java
      if (in.getSelected() != null && in.getSelected() > 0 && in.getSelected() < 11) {
        if (in.getCoins() != null && in.getCoins().size() > 0 && in.getCoins().size() < 101) {
 
          // 残りのコード
 
        } else {
          throw new IllegalArgumentException();
        }
      } else {
        throw new IllegalArgumentException();
      }

最初に検出したエラーを処理するコードが最後まで探さないと見付かりません。

これだと見にくいので、
「if では “例外的な処理” あるいは “簡単に済ますことのできる処理” を先に持ってくる」という鉄則を適用します。

 LANG:java
      if ( !(in.getSelected() != null && in.getSelected() > 0 && in.getSelected() < 11) ) {
        throw new IllegalArgumentException();
      }
 
      if ( !(in.getCoins() != null && in.getCoins().size() > 0 && in.getCoins().size() < 101) ) {
        throw new IllegalArgumentException();
      }
 
      // 残りのコード

ネストが２段減りました。

短い処理を先に済ませてしまえば、
大きなコードブロックを呼んでいる時にはもう忘れて構わないので、
脳内記憶領域を圧迫せずに済みます。

https://gist.github.com/osamutake/eef52076810a996831c6/384a83b236606173b330febff3f1ccb792fe5278

* 引数チェック部分を関数として切り出す [#t6dcb8e8]

上記の２つの if の中身について、
何をやっているかが解りやすい形で関数に切り出そうとしたところ、こうなりました。

 LANG:java
      if ( !IsSelectedInRange(in.getSelected(), 1, 10) ||
           !IsCoinCountInRange(in.getCoins(), 1, 100) ) {
        throw new IllegalArgumentException();
      }

関数定義はこう。

 LANG:java
  boolean IsSelectedInRange(Integer selected, int from, int to)
  {
    return selected != null && from <= selected  && selected <= to;
  }
  
  boolean IsCoinCountInRange(List<Integer> coins, int from, int to) 
  {
    return coins != null && from <= coins.size() && coins.size() <= to;
  }

それぞれ &math(0<x<11); や &math(0<y<101); よりも、
&math(1\leqq x\leqq 10); や &math(1\leqq y\leqq 100); が意図するところだと判断しました。

また、範囲を指定する数値をサブ関数に埋め込まない方がメインロジックの処理を追いやすいように思えたため、引数として渡すようにしてあります。

同じ範囲に対するチェックをあちこちで使い回すのであれば、
範囲の両端の定数まで関数側に含めた方が良いかもしれません。

もちろん、定数はコード中に数字を生で書かず、どこかにまとめて設定すべきなのですが、
ここではまとめる場所がないのでそのままになっています。

https://gist.github.com/osamutake/eef52076810a996831c6/9da45cf27ae0662045896f947a9981ba67d81143

* coins2, sum の意味を考える [#x8569c25]

コードのやっていることは・・・

- coins の中から１円玉と５円玉を取り出して coins2 に入れている
- 残った１０円～５００円玉の合計を sum に入れている

でしょうか。そこで、それぞれ

- coins2 → smallCoins
- sum → sumOfLargeCoins

という名前に付け替えました。

特定の意味を持たない番号付きの名前が付けられた変数 coins2 なんてのは、
小さいながらもコードの臭いの元になるので、
早めにちゃんとした名前を付けてあげたくなります。

https://gist.github.com/osamutake/eef52076810a996831c6/59c285cd358fd5c570160215efad35e82e5746de

* smallCoins, sumOfLargeCoins を求める計算を解りやすくする [#m2b9bb59]

この部分、やっていることが理解できたので、
分かりやすい書き方に直します。

 LANG:java
      List<Integer> smallCoins = new ArrayList<>();
      int sumOfLargeCoins = 0;
      for (Integer coin : in.getCoins()) {
        if (coin == 1) {
          sumOfLargeCoins += 0;
          smallCoins.add(coin);
        } else if (coin == 5) {
          sumOfLargeCoins += 0;
          smallCoins.add(coin);
        } else if (coin == 10) {
          sumOfLargeCoins += 10;
        } else if (coin == 50) {
          sumOfLargeCoins += 50;
        } else if (coin == 100) {
          sumOfLargeCoins += 100;
        } else if (coin == 500) {
          sumOfLargeCoins += 500;
        }
      }

これを、

 LANG:java
      List<Integer> smallCoins = new ArrayList<>();
      int sumOfLargeCoins = 0;
      for (Integer coin : in.getCoins()) {
        if (coin == 1 || coin == 5) {
          smallCoins.add(coin);
        } else
        if (coin == 10 || coin == 50 || coin == 100 || coin == 500 ) {
          sumOfLargeCoins += coin;
        } else {
          // throw new IllegalArgumentException();
        }
      }

こう。

small coin と large coin との場合分けであることがはっきりしました。

どちらでもない場合の処理が抜け落ちていることも発見しました。
例外を投げるなりのエラー処理が必要な気配があります。

この部分を関数に切り出しても良いのですが、
smallCoins と sumOfLargeCoins のどちらを求めることを主にして関数名を付けたらいいのか
名前付けを迷います。
あるいはいっそ、それぞれを求める２つの関数に分けてしまった方がいいかもしれないとも考えるのですが、
その場合、上で throw new IllegalArgumentException(); とした else の節をどちらに含めたらいいか迷ってみたり。。。
どうも方針が決まらないのでとりあえずこのまま進みます。

意味を良く理解しないまま、おかしな名前で関数を切り出してしまうと、メインのロジックが追いにくくなる問題もあると思います。

https://gist.github.com/osamutake/eef52076810a996831c6/b19382b0e1e5b11728a3fa2f77fb7b621410d0da

* result の作成、return を局所的にする [#u73e38a6]

現状のコード、result についてだけ見ると、

 LANG:java(linenumber)
  public Result execute(Input in) {
    Result result = new Result();
    try {
      if ( ... ) {
        throw new IllegalArgumentException();
      }
 
      if ( ... ) {
        result = new Result();
        result.setCoins(in.getCoins());
        result.setProduct(null);
      } else if ( ... ) {
        result.setProduct(product);
        ...
        result.setCoins(smallCoins);
      } else {
        result = new Result();
        result.setCoins(in.getCoins());
        result.setProduct(null);
      }
    } catch (Exception e) {
      LOG.error(e);
      result = new Result();
      result.setCoins(in.getCoins());
      result.setProduct(null);
    }
 
    return result;
  }

result を作るコードがあちこちに分散してしまっていて見づらいです。
しかもロジック的には result を作成したらそのまま関数を抜けているにもかかわらず、
そのことが関数を最後まで読まないと分からない構造になっています。

そこで、result を作るそれぞれのコードの直後に return result; を入れて、
関数から抜けることを明示しました。

これにより、if のネストがさらに減る効果もあります。

また、上で 9行目、17行目、23行目に余計な new Result() があったのも、実質的にここで取り除かれたことになります。

ここまでのコードはこんな感じ。

#gist(osamutake/eef52076810a996831c6/546d8eadd94f9cfe3d2059f68feeba7856473c23);

* Product の無いときの処理を createResultWitoutProduct にまとめた [#p63f9043]

 LANG:java
      Result result = new Result();
      result.setCoins(in.getCoins());
      result.setProduct(null);
      return result;

という処理が３カ所に現れていたため、

 LANG:java
  Result createResultWitoutProduct(List<Integer> coins)
  {
    result = new Result();
    result.setCoins(coins);
    result.setProduct(null);
    return result;
  }

を作って、

 LANG:java
      return createResultWitoutProduct(in.GetCoins());

に書き換えました。

https://gist.github.com/osamutake/eef52076810a996831c6/fc5bca3fb011785837a780058ebca0ae7d602bed

* product に関する例外的な処理を先頭に移した [#c9777b9d]

 LANG:java
      Product product = dao.findById(in.getSelected());
      if (product == null) {
        return createResultWitoutProduct(in.getCoins());
      } 
 
      if (product.getPrice() < sumOfLargeCoins) {
        // 大きなコードブロック
        return result;
      } 
      
      return createResultWitoutProduct(in.getCoins());

を

 LANG:java
      Product product = dao.findById(in.getSelected());
      if ( product == null || product.getPrice() >= sumOfLargeCoins ) {
        return createResultWitoutProduct(in.getCoins());
      }
 
      // 大きなコードブロック
      return result;

とすることで、if のネストも減らせています。

https://gist.github.com/osamutake/eef52076810a996831c6/ad721397142e46aeafb123777f96a93c200e545a

* coins3 および、書き換えられた後の sumOfLargeCoins の意味を考えた [#t0980957]

それぞれ、

- coins3 → largeCoinList
- sumOfLargeCoins → remainder

としてみました。

２つ目の remainder への書き換えは、「同じ変数を異なる意味で使わない」という意図があります。
「sum から値を引いていく」というコードだけ見ても、何やらぷんぷん臭ってくる訳で。。。

同時に、largeCoinList を作成するコードを関数として切り出しました。
本来ならこれは定数として準備しておけばよい物なのですが、
この execute 関数の周りのコードがどうなっているのか解らないため、
さしあたり関数に切り出すことで満足しておきます。

元のコードで、

 LANG:java
      Result result = new Result();
      result.setProduct(product);
      sumOfLargeCoins -= product.getPrice();
 
      List<Integer> coins3 = new ArrayList<>();
      coins3.add(500);
      coins3.add(100);
      coins3.add(50);
      coins3.add(10);
      for (Integer coin : coins3) {
        while (sumOfLargeCoins > coin) {
          smallCoins.add(coin);
          sumOfLargeCoins -= coin;
        }
      }
      result.setCoins(smallCoins);
      return result;

の部分が

 LANG:java
      Result result = new Result();
      result.setProduct(product);
      int remainder = sumOfLargeCoins - product.getPrice();
 
      List<Integer> largeCoinList = getLargeCoinList();
      for (Integer coin : largeCoinList) {
        while (remainder > coin) {
          smallCoins.add(coin);
          remainder -= coin;
        }
      }
      result.setCoins(smallCoins);
      return result;

になりました。

https://gist.github.com/osamutake/eef52076810a996831c6/857dd2ea5f9572c795c95106d222f19dab477d27

* 内容が理解できたので変数名、関数名を適切な物に付け替えた [#g361403f]

SIZE(30){COLOR(red){ここが最大のネタバレポイントです。}}

上記のコードでは smallCoins に largeCoinList に含まれる coin を追加していたりして、
明らかに意味を取り違えているのが解ります。

でも、そうして作った smallCoins を product と共に Result に渡して終了するという
ところまで読んで、ようやくコード全体の目的が見えてきました。

そこで、その他も含めて変数名や関数名を適切な物に付け替えました。

- smallCoins → coinsToReturn
- sumOfLargeCoins → deposit 
- remainder → deposit
- getLargeCoinList → getCoinListAllowedForChange

一旦、sum という変数を sumOfLargeCoins と remainder に分けましたが、
結局は deposit という名前で１つに戻りました。
それと、large coins と small coins という、何に比べて大きいのか、小さいのかがあいまいだった
これらの概念も消失しています。

名前の付け方、本当に大事です。

名前を付け替えたことで、コードを見るだけで処理内容が読み取れるようになりました、、、よね？

#gist(osamutake/eef52076810a996831c6/ed36b093b4f878885a40b994e855d784c0e4e9a3);

たぶんこの execute という関数は、VendingMachine というクラスのメンバーで、
onProductButtonClick というイベントハンドラから呼び出されるんじゃないかと想像できます。

まあ、実際の自販機では１円玉や５円玉は機械に読み取られず、即座に落ちてくるわけですし、
コインの投入毎に deposit を更新して電光掲示板に表示するはずなのですけどね。

* 入金額の計算部分を関数に切り出した [#n0bf50de]

こうなれば、上でペンディングにした deposit の計算部分を関数に切り出すのも問題なく行えます。

 LANG:java
      List<Integer> coinsToReturn = new ArrayList<>();
      int deposit = calcDeposit(in.getCoins(), coinsToReturn);

https://gist.github.com/osamutake/eef52076810a996831c6/d87944c57b81d925e324144a4d014eac1d01e254

* in.getSelected(), in.getCoins() をまとめる [#w556bd31]

何を取り出しているか解りにくい名前なので、
冒頭で説明的な名前の変数に受けることにしました。

 LANG:java
      Integer productId = in.getSelected();
      List<Integer> coinsInserted = in.getCoins();

https://gist.github.com/osamutake/eef52076810a996831c6/387e1a62d59b245d231ed7401f3e6f8acf8ca1bd

* おつりの計算も関数に切り出した [#v574c0ac]

 LANG:java
      deposit -= product.getPrice();
      calcChange(deposit, coinsToReturn);

https://gist.github.com/osamutake/eef52076810a996831c6/6be2a25613a7cf6cc93e0df3777c89f3680c6e8a

~
~
~
~
COLOR(red){SIZE(30){次も大きなネタバレっぽいです。}}

~
~
~
~
~
~
~
~
~
~
~

* 重要な修正 [#o6e43bcb]

ここまでくると元のコードに１つ明らかなバグがあったことに気づけます。

 LANG:java
      if ( product == null || product.getPrice() >= deposit ) {
        return createResultWitoutProduct(coinsInserted);
      }

これだと入金額ぴったりの金額の商品を買えません。

正しくは、

 LANG:java
      if ( product == null || product.getPrice() > deposit ) {
        return createResultWitoutProduct(coinsInserted);
      }

なのだと思います。

https://gist.github.com/osamutake/eef52076810a996831c6/374bb12ae56cbb083616dbe0d521cfdd5ba449b6

* getCoinListAllowedForChange は名前が悪い [#ye523ef7]

うっかりしていたのですが getCoinListAllowedForChange の返すリストは
値の大きいものから順に並んでいないとバグを生むにもかかわらず、
この関数名からはそれが読み取れません。

名前を getValidCoinsDescending と変えれば calcChange のアルゴリズムがはっきりしますし、

 LANG:java
  void calcChange(int deposit, List<Integer> coinsToReturn)
  {
    for (Integer coin : getValidCoinsDescending()) {
      while (deposit > coin) {
        coinsToReturn.add(coin);
        deposit -= coin;
      }
    }
    // if (deposit != 0) {
    //   // product.Price() が 10 で割り切れない！
    //   throw new IllegalArgumentException("product with illegal price found");
    // }
  }

同じ関数を calcDeposit の中でも使えそうです。

 LANG:java
  int calcDeposit(List<Integer> coins, List<Integer> coinsToReturn)
  {
    int deposit = 0;
    List<Integer> validCoins = getValidCoinsDescending();
    for (Integer coin : coins) {
      if (validCoins.contains(coin)) {
        deposit += coin;
      } else {
        coinsToReturn.add(coin);
      }
    }
    return deposit;
  }

COLOR(RED){SIZE(30){ここは元のコードからの仕様変更を含んでいます。}}

考えてみれば、10, 50, 100, 500 円以外は 1, 5 円以外の硬貨があったとしても返してしまえばいいので、
例外を上げる必要はなさそう、という判断による書き換えです。

2円硬貨とか120円硬貨、10万円硬貨なんかが渡されるのはやはり何かおかしいのでエラーにすべき、
というのもあるのですが、もともとエラーを処理する catch の処理も、
どんな硬貨も Result に渡せば返せることを前提としているので、
ここでも知らん顔して返してしまいます。

改善前のコードでは想定外の硬貨は自販機に飲み込まれてしまうので、
それに比べればずっといいはずです。

https://gist.github.com/osamutake/eef52076810a996831c6/14b0d57bd8b82f3e06a1321ab544eaedbdcca72c

* isProductAvailable を切り出した [#i58dd0f9]

今更ですが、

 LANG:java
      if ( product == null || product.getPrice() > deposit ) {
        return createResultWitoutProduct(coinsInserted);
      }

を次のように直しました。

 LANG:java
      if (!IsProductAvailable(product, deposit)) {
        return createResultWitoutProduct(coinsInserted);
      }

https://gist.github.com/osamutake/eef52076810a996831c6/cda66c048d47b0159070e8262e9675821f7425e6

* throw する例外にはちゃんとメッセージを残すべき [#r69d4e43]

考えてみれば当たり前。

でもそうすると行数が増えるので、関数に切り出した。
あと、typo も直した。

 LANG:java
  void checkInput(int productId, List<Integer> coinsInserted)
  {
    if ( !isProductIdInRange(productId, 1, 10) ) {
      throw new IllegalArgumentException("productId out of range");
    }
    
    if ( !isCoinCountInRange(coinsInserted, 1, 100) ) {
      throw new IllegalArgumentException("coin count out of range");
    }
  }

こっちもメッセージを入れておく。

 LANG:java
    //   throw new IllegalArgumentException("product with illegal price found");

https://gist.github.com/osamutake/eef52076810a996831c6/575e26c06a0eabc92807f12a36bfeace776bebdb

* 改善されたコード [#w9615ff8]

#gist(osamutake/eef52076810a996831c6);

あとは、何かあるかなあ・・・

+ getValidCoinsDescending  の中が格好悪いのだけれど、Java をよく知らないので直せない。
-- そもそもコメントの通り定数値を返せるようにすべきだし。
+ in.getSelected は Input の文脈では自明なのかもしれないけれど、
このコードの中では in.getSelectedProductId という名前の方がより解りやすい。
+ IsProductIdInRange や IsCoinCountInRange で便利に使える Range クラスとかあるならそれを使ってもいい。
この程度のコードなら無理に使わなくても良いけれど。
+ calcChange 内のコメントアウトされた throw は適切な例外を投げるべき。いざというときにお客さんに損をさせることになるので念のため。
+ createResult(product, coins) という関数を作っても良さそう。ただそれならむしろ new Result(product, coins) というコンストラクタを用意すれば良いのだけれど、そうすると 「コード改善の例題」 としてヒントが少なすぎちゃうから明示的に setProduct, setCoins を呼ぶ形にしたという親心なのかも？
+ うへ、typo がある createResultWitoutProduct -> createResultWithoutProduct

とかとか。

* in.getSelected() の戻り値 [#ea93d0b3]

もしかすると、in.getSelected() の戻り値は producId ではなく brandId なのかもしれないですね。

そうでないと在庫チェックのコードがどこにも見あたらないことになってしまう。

dao にはいろんな種類の商品の在庫が入っていて、押されたボタンに対応する種類の在庫があれば
商品を１つ取り出して返す。product.id は商品毎に異なる値を取り、product.brand.id == in.getSelected() 
になる。

という解釈が現実的な気がしてきました。

in.getSelected() の戻り値が範囲外であればログに残すのに（システムエラー）、
dao.findById() == null をログに残さないこととも整合します（単なる在庫切れ）。

ただそれならば dao.findById という名前は紛らわしくて、dao.findByBrandId だし、
getPrice は product ではなく brand にあるべきかだし、
あんまり自信が無いので現状はそのままにしておくことにします。

＃あれ、でも brandId が１～１０というのは少なすぎるか

** そうじゃなくて [#s5221b79]

in.getSelected() の返り値を selectedButtonId あるいは selectedTruckId 
だと考えるといろいろしっくり来るかもしれません？

dao.findById() はボタンに対応する在庫があることを確認して、~
無ければ null を、~
あれば button クラスの実体を返す。

button.getPrice() は問題ないし、

返った Result を処理する部分でも、ボタンに対応する列から商品を１つ出せば良いことになる。

いやいやいやいや、dao.findById() の戻り値が Product なのは確定なのか・・・

しかも、在庫切れのボタンはそもそも「売り切れ」を表示して押せないようにすべきなので、
dao.findById() で null が返るのはやはりおかしいのかも。

なんで dao.findById() == null に対してはログを取ってないんでしょうね？？？

始めは dao.findById() == null はシステムエラーではなく単なる在庫切れなのかと思ったのですが、
いろいろ考えている内によく分からなくなってきてしまいました。

** いずれにしても [#t10b35d6]

元のコードを書いた人の意図が分からない状況では、
dao.findById() == null に対して例外を発生し、ログに残すような「仕様変更」は
しないほうが良いと思います。

VendingMachine が溜められるログの記憶領域は非常に限られているかもしれません。
その他にも、それが単なる在庫切れを表わしているなど、ログを取らない理由があるのかもしれません。
どんなエラーでログを取るか、どんなエラーでログを取らないか、は、
厳密に仕様として決まっているべきで、コードを書いた人に確かめられない現状では、
元のコードは仕様通りに作られていることを前提にすべきです。

そういうことを考えた上で、「product.getPrice() の値が 10 で割り切れない場合」については
ログを残すべきだと判断します。元のコードでまるっきりチェックから漏れていた事実から、
これをログに残さないという仕様があるとは思えないこと、ログに残すのに値する、
非常にまずいことが起きているのが確実であること、がその理由です。

＃ 在庫確認にこだわってたけど、本来は釣り銭の残り状況確認なんかも必要なはずで
＃ そう考えると気にする必要は無いのかも

* 本家の更新を見て手直し１ [#q0ce5217]

[[続々々々々々：僕の好きなコードの書き方>http://bufferings.hatenablog.com/entry/2015/06/02/225224]]
を見て、上記のコードを手直し。

現在私のコードは、

 LANG:java
       List<Integer> coinsToReturn = new ArrayList<>();
       int deposit = calcDeposit(coinsInserted, coinsToReturn);
       ...
       deposit -= product.getPrice();
       calcChange(deposit, coinsToReturn);

のようになっていて、
- calcDeposit が deposit と coinsToReturn を計算している
- calcChange がおつりを計算して coinsToReturn に加えている

のように、２つの関数ともそれぞれ複数の役割を果たしてしまっています。

これを、

 LANG:java
       List<Integer> coinsRefused = new ArrayList<>();
       int deposit = calcDeposit(coinsInserted, coinsRefused);
       ...
       deposit -= product.getPrice();
       List<Integer> coinsForChange = calcChange(deposit);
       List<Integer> coinsToReturn = ListUtils.union(coinsRefused, coinsForChange);

このように直すことで、
- calcDeposite は入金額を計算して、受け入れられなかったコインを coinsRefused に入れる
- calcChange はおつりを計算する
- coinsToReturn は coinsRefused と coinsForChange を合わせた物

と、役割がはっきりします。

確かにこの方がずっといい。

一方で、deposit を計算する手順と coinsRefused を計算する部分とは、むしろ分けたくないと思っています。
これらを離ればなれにしてしまうと、意味の関係性が薄れてしまうように感じるためです。

https://gist.github.com/osamutake/eef52076810a996831c6/446d3f99daeaa969a0df1991a622eaccda61a8f1

* 本家の更新を見て手直し２ [#s4cd9a3b]

[[続々々々々々：僕の好きなコードの書き方>http://bufferings.hatenablog.com/entry/2015/06/02/225224]]
を見て、私のコードをもう一つ手直し。

 LANG:java
  public Result execute(Input in) {
    try {
      Integer productId = in.getSelected();
      List<Integer> coinsInserted = in.getCoins();
      ...
       
    } catch (Exception e) {
      LOG.error(e);
      return createResultWitoutProduct(coinsInserted);
    }
  }

これは、ダメだ。

ほぼ無さそうで、無視したくはなるけれど、
in.getSelected() で例外が発生すると、
in.getCoins() には値が入っているのに
coinsInserted には値が入っていない状況になりかねない。

順番を入れ替えて、

 LANG:java
  public Result execute(Input in) {
    try {
      List<Integer> coinsInserted = in.getCoins();
      Integer productId = in.getSelected();

とすれば現時点では解決だけれど、
将来的にコードが増えたりすることを考えると素直に

 LANG:java
  public Result execute(Input in) {
    try {
      Integer productId = in.getSelected();
      List<Integer> coinsInserted = in.getCoins();
      ...
       
    } catch (Exception e) {
      LOG.error(e);
      List<Integer> coinsInserted = in.getCoins();
      return createResultWitoutProduct(coinsInserted);
    }
  }

とすべきですね。

＃というか、元のコードだとそもそも catch 内では coinsInserted がスコープ外なのか（汗

https://gist.github.com/osamutake/eef52076810a996831c6

* 最新版 [#vac343e5]

#gist(osamutake/eef52076810a996831c6);


* 本家の進行状況についていくつか感想を [#j21199db]

これらのうち、「突っ込み」になっているものの多くは、
私とMitsuyuki.Shiibaさんとで想定しているコード改善の状況が異なっていたことによるものでした。
私は「汚いけど一応動いているコード」を改善しているつもりで書き直していたのですが、
Mitsuyuki.Shiiba さんはプロトタイピングの段階を想定されていたのでした。

- [[続々々々：僕の好きなコードの書き方>http://bufferings.hatenablog.com/entry/2015/05/27/090941]]
-- NPE チェックを try の外で行うようにしたことで、NPE がログに残らなくなるという「仕様変更」が生じています。この段階で仕様を変えるような変更はちょっと怖いと思います。
-- intValue() を呼ばなければならないあたり、私のコードには Java を知らないことが理由のいろんな抜けがありそうですね
-- 「Productが見つからない場合も、例外投げる。」これもログに残す内容に対する仕様変更になっています。
この段階ではもう少し慎重になるべきではないでしょうか。
- [[続々々々々：僕の好きなコードの書き方>http://bufferings.hatenablog.com/entry/2015/05/28/090652]]
-- Preconditions を使ったことにより、例外の種類が変わりました？
ログの取り方にもよりますが、これも小さな仕様変更になる可能性がありますね。
-- 値が範囲外であることと null の検出とでメッセージを分けているのはその方が良いのかも。コードの増加に釣り合うなら。
--  あれ？ [[CollectionUtils.isNotEmpty>https://commons.apache.org/proper/commons-collections/javadocs/api-3.2.1/org/apache/commons/collections/CollectionUtils.html#isNotEmpty%28java.util.Collection%29]] が null を返すことってあるのかしら？？？バグっぽいです。
-- この関数の役割が判明してからの判断になるかもしれませんが・・・
この関数については入力値のチェックは最低限にすべきだと思います。
なぜならこの関数はアプリケーションのあちこちから呼ばれるような汎用の関数ではなくて、ほぼ一カ所からしか呼ばれない関数だと思われるからです。in の null チェックは不要で、恐らく本来なら getSelected、getCoins の null チェックも不要でしょう。機器の設定ミスなど、何らかの原因で起きかねないエラーの検出以外は無用な複雑さ（バグを生む可能性を増やす）の導入にしかならないと思います。例えば b( ) の頭で入力値のチェックをしないのは a( ) でチェック済みであるからなのですが、それと同様に execute( ) を呼び出す側のコードで in == null のまま呼び出してしまう状況はありえないだろう、という話です。
-- 動作しないコードで日をまたぐのであれば、その旨を「コードに」コメントとして残しておきたいです。
--- その際、"TODO: processCoins2でreturnCoins計算する。" だと必要な内容が伝わらない心配が。
-- 全体的に、Java っていろいろ進化してるんだなあ、などという本題とは関係のないところに感心してます
--- ラムダ式とか、ジェネリックの代入における型変換？型推論？とか。
-- あと全体的に、コードの動作を変えることを恐れない雰囲気が伝わってきて、疑似リファクタリングのつもりで読んでいると、ちょっとハラハラします。
- [[続々々々々々：僕の好きなコードの書き方>http://bufferings.hatenablog.com/entry/2015/06/02/225224]]
-- invalidCoins と changeCoins を別々に計算してあとで union するのは是非そうすべきですね。私のコードも直したいと思います。
-- ありゃ、私のコード、catch にて insertedCoins を参照してますが、getSelected でこけたりすると、
必ずしも設定されているとは限りませんね。明らかにバグなので直します。
- [[まとめ：僕の好きなコードの書き方>http://bufferings.hatenablog.com/entry/2015/06/03/082323]]
-- そうか、私は「汚いけど一応動いているコード」を改善しているつもりで書き直していたのですが、
Mitsuyuki.Shiiba さんはプロトタイピングの雰囲気だったのですね。そのスタンスの違いがコードの書き方の
違いにも現れていたのだなあと、いろいろ納得しました。

* コメント [#t914c9c4]

#article_kcaptcha
Counter: 7971 (from 2010/06/03), today: 1, yesterday: 3