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

更新

[[公開メモ]]

* 概要 [#f7d6579a]

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

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

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

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

- 僕の好きなコードの書き方 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

** ネタバレ [#qbbe22f5]

COLOR(red){SIZE(26){Mitsuyuki.Shiiba さんの記事をリアルタイムでワクワク読みたい方は、これ以降はまだ読まない方がいいです。}}

COLOR(red){SIZE(26){完全にネタバレになってます。}}

お題が非常に面白かったので先走ってしまいました。
申し訳ありません。
** このページの目次 [#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();
      }
 
      // 残りのコード

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

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

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

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

何をやっているかが解りやすい形で切り出そうとしたところ、こうなりました。

 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 の中から1円玉と5円玉を取り出して coins2 に入れている
- 残った10円~500円玉の合計を sum に入れている

そこで、それぞれ

- coins → smallCoins
- sum → sumOfLargeCoins

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

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

こう。

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

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

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

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

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

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

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

また、元の 29行目、49行目、61行目に余計な new Result() があったのも、実質的にここで取り除かれたことになります。

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

#gist(osamutake/eef52076810a996831c6/546d8eadd94f9cfe3d2059f68feeba7856473c23);

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

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

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

 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

としてみました。

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

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

 LANG:java
      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;
        }
      }

の部分が

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

になりました。

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 という名前で1つに戻りました。名前の付け方、本当に大事です。

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

#gist(osamutake/eef52076810a996831c6/ed36b093b4f878885a40b994e855d784c0e4e9a3);

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

まあ、実際の自販機では1円玉や5円玉は機械に読み取られず、即座に落ちてくるわけですけどね。
* 入金額の計算部分を関数に切り出した [#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]

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

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

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

正しくは、

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

なのだと思います。

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

考えてみれば、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 は適切な例外を投げるべき。いざというときにお客さんに損をさせることになるので念のため。
+ もしかすると、in.getSelected() の戻り値は producId ではなく brandId なのかもしれない。
dao にはいろんな種類の商品の在庫が入っていて、押されたボタンに対応する種類の在庫があれば
商品を1つ取り出して返す。product.id は商品毎に異なる値を取り、product.brand.id == in.getSelected() 
になるという解釈の方が現実的な気がしてきた。ただそれならば dao.findById ではなく dao.findByBrandId であるべきだ。
+ createResult(product, coins) という関数を作っても良さそう。ただそれならむしろ new Result(product, coins) で良いはずなのだけれど、そうすると問題としてヒントが少なすぎちゃうから明示的に setProduct, setCoins を呼ぶ形にしたという親心?

とかとか。

* コメント [#t914c9c4]

#article_kcaptcha

Counter: 7433 (from 2010/06/03), today: 4, yesterday: 0