プログラミング/作法/Hello.java のバックアップ(No.12)

概要 †

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

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

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

オリジナルのソース †

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

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

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

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

引数チェック部分を関数として切り出す †

上記の２つの 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;
 }

それぞれ や よりも、 や が意図するところだと判断しました。

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

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

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

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

coins2, sum の意味を考える †

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

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

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

• coins2 → smallCoins
• sum → sumOfLargeCoins

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

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

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

smallCoins, sumOfLargeCoins を求める計算を解りやすくする †

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

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 を局所的にする †

現状のコード、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() があったのも、実質的にここで取り除かれたことになります。

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

Product の無いときの処理を createResultWitoutProduct にまとめた †

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 に関する例外的な処理を先頭に移した †

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 の意味を考えた †

それぞれ、

• 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

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

ここが最大のネタバレポイントです。

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

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

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

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

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

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

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

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

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

入金額の計算部分を関数に切り出した †

こうなれば、上でペンディングにした 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() をまとめる †

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

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

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

おつりの計算も関数に切り出した †

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

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

次も大きなネタバレっぽいです。

重要な修正 †

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

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 は名前が悪い †

うっかりしていたのですが 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;
 }

ここは元のコードからの仕様変更を含んでいます。

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

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

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

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

isProductAvailable を切り出した †

今更ですが、

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 する例外にはちゃんとメッセージを残すべき †

考えてみれば当たり前。

でもそうすると行数が増えるので、関数に切り出した。 あと、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

改善されたコード †

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

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

とかとか。

in.getSelected() の戻り値 †

もしかすると、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 が１～１０というのは少なすぎるか

本家の進行状況についていくつか感想を †

• 続々々々：僕の好きなコードの書き方
  • NPE チェックを try の外で行うようにしたことで、NPE がログに残らなくなるという「仕様変更」が生じています。この段階で仕様を変えるような変更はちょっと怖いと思います。
  • intValue() を呼ばなければならないあたり、私のコードには Java を知らないことが理由のいろんな抜けがありそうですね
  • 「Productが見つからない場合も、例外投げる。」これもログに残す内容に対する仕様変更になっています。 この段階ではもう少し慎重になるべきではないでしょうか。
• 続々々々々：僕の好きなコードの書き方
  • Preconditions を使ったことにより、例外の種類が変わりました？ ログの取り方にもよりますが、これも小さな仕様変更になる可能性がありますね。
  • 値が範囲外であることと null の検出とでメッセージを分けているのはその方が良いのかも。コードの増加に釣り合うなら。
  • あれ？ CollectionUtils.isNotEmpty が null を返すことってあるのかしら？？？バグっぽいです。
  • この関数の役割が判明してからの判断になるかもしれませんが・・・ この関数については入力値のチェックは最低限にすべきだと思います。 なぜならこの関数はアプリケーションのあちこちから呼ばれるような汎用の関数ではなくて、ほぼ一カ所からしか呼ばれない関数だと思われるからです。in の null チェックは不要で、恐らく本来なら getSelected、getCoins の null チェックも不要でしょう。機器の設定ミスなど、何らかの原因で起きかねないエラーの検出以外は無用な複雑さ（バグを生む可能性を増やす）の導入にしかならないと思います。例えば b( ) の頭で入力値のチェックをしないのは a( ) でチェック済みであるからなのですが、それと同様に execute( ) を呼び出す側のコードで in == null のまま呼び出してしまう状況はありえないだろう、という話です。
  • 動作しないコードで日をまたぐのであれば、その旨を「コードに」コメントとして残しておきたいです。
    • その際、"TODO: processCoins2でreturnCoins計算する。" だと必要な内容が伝わらない心配が。
  • 全体的に、Java っていろいろ進化してるんだなあ、などという本題とは関係のないところに感心してます
    • ラムダ式とか、ジェネリックの代入における型変換？型推論？とか。
  • あと全体的に、コードの動作を変えることを恐れない雰囲気が伝わってきて、疑似リファクタリングのつもりで読んでいると、ちょっとハラハラします。

コメント †