プログラミング/作法/Hello.java の履歴(No.7)
更新概要†
Mitsuyuki.Shiiba さんという方が、「僕の好きなコードの書き方」というタイトルで ブログを書いてらっしゃって、その中でちょっとあれなコードを公開でリファクタリングしていたのですが、 例として挙げられたコードが秀逸だったので自分でもリファクタリングにトライしてみました*1本当はテストのない状態でのコード改善をリファクタリングと呼んではいけません。よい子の皆さんはまねしないで下さい。 。
オリジナルのコードが Gist 上に https://gist.github.com/bufferings/9414995751687177e882 にあったのですが、特定のリビジョンから Fork する方法が解らなくて、私の方でコピペで新しい Gist を作ることになってしまいました。たぶん間違った方法なんだと思います。どうもすみません。
ここからも解るように、私は Gist も、実は Java もあまりよく分かっていません。普段は Ruby とか JavaScript とか C# とか良く書いてます。Java のコンパイル環境もなしでやるので、タイプミスについてもご容赦下さい。
本家の記事更新状況†
- 僕の好きなコードの書き方 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
ネタバレ†
Mitsuyuki.Shiiba さんの記事をリアルタイムでワクワク読みたい方は、これ以降はまだ読まない方がいいです。
完全にネタバレになってます。
お題が非常に面白かったので先走ってしまいました。 申し訳ありません。
このページの目次†
- 概要
- オリジナルのソース
- 6 行目, 7 行目の if の連鎖によりコードのインデントが深くなってしまっている。
- 引数チェック部分を関数として切り出す
- coins2, sum の意味を考える
- smallCoins, sumOfLargeCoins を求める計算を解りやすくする
- result の作成、return を局所的にする
- Product の無いときの処理を createResultWitoutProduct にまとめた
- product に関する例外的な処理を先頭に移した
- coins3 および、書き換えられた後の sumOfLargeCoins の意味を考えた
- 内容が理解できたので変数名、関数名を適切な物に付け替えた
- 入金額の計算部分を関数に切り出した
- in.getSelected(), in.getCoins() をまとめる
- おつりの計算も関数に切り出した
- 重要な修正
- getCoinListAllowedForChange は名前が悪い
- isProductAvailable を切り出した
- throw する例外にはちゃんとメッセージを残すべき
- 改善されたコード
- in.getSelected() の戻り値
- コメント
オリジナルのソース†
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(); } // 残りのコード
ネストが2段減りました。
短い処理を先に済ませてしまえば、 大きなコードブロックを呼んでいる時にはもう忘れて構わないので、 脳内記憶領域を圧迫せずに済みます。
https://gist.github.com/osamutake/eef52076810a996831c6/384a83b236606173b330febff3f1ccb792fe5278
引数チェック部分を関数として切り出す†
上記の2つの 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 の中から1円玉と5円玉を取り出して coins2 に入れている
- 残った10円~500円玉の合計を 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 のどちらを求めることを主にして関数名を付けたらいいのか 名前付けを迷います。 あるいはいっそ、それぞれを求める2つの関数に分けてしまった方がいいかもしれないとも考えるのですが、 その場合、上で throw new IllegalArgumentException(); とした else の節をどちらに含めたらいいか迷ってみたり。。。 どうも方針が決まらないのでとりあえずこのまま進みます。
意味を良く理解しないまま、おかしな名前で関数を切り出してしまうと、メインのロジックが追いにくくなる問題もあると思います。
https://gist.github.com/osamutake/eef52076810a996831c6/b19382b0e1e5b11728a3fa2f77fb7b621410d0da
result の作成、return を局所的にする†
result を作るコードがあちこちに分散してしまっていて見づらいです。 しかもロジック的には result を作成したらそのまま関数を抜けているにもかかわらず、 そのことが関数を最後まで読まないと分からない構造になっています。
そこで、result を作るそれぞれのコードの直後に return result; を入れて、 関数から抜けることを明示しました。
これにより、if のネストがさらに減る効果もあります。
また、元の 29行目、49行目、61行目に余計な new Result() があったのも、実質的にここで取り除かれたことになります。
ここまでのコードはこんな感じ。
Product の無いときの処理を createResultWitoutProduct にまとめた†
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 に関する例外的な処理を先頭に移した†
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
としてみました。
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
内容が理解できたので変数名、関数名を適切な物に付け替えた†
ここが最大のネタバレポイントです。
上記のコードでは smallCoins に largeCoinList に含まれる coin を追加していたりして、 明らかに意味を取り違えているのが解ります。
でも、そうして作った smallCoins を product と共に Result に渡して終了するという ところまで読んで、ようやくコード全体の目的が見えてきました。
そこで、その他も含めて変数名や関数名を適切な物に付け替えました。
- smallCoins → coinsToReturn
- sumOfLargeCoins → deposit
- remainder → deposit
- getLargeCoinList → getCoinListAllowedForChange
一旦、sum という変数を sumOfLargeCoins と remainder に分けましたが、 結局は deposit という名前で1つに戻りました。名前の付け方、本当に大事です。
名前を付け替えたことで、コードを見るだけで処理内容が読み取れるようになりました、、、よね?
たぶんこの execute という関数は、VendingMachine というクラスのメンバーで、 onProductButtonClick というイベントハンドラから呼び出されるんじゃないかと想像できます。
まあ、実際の自販機では1円玉や5円玉は機械に読み取られず、即座に落ちてくるわけですけどね。
入金額の計算部分を関数に切り出した†
こうなれば、上でペンディングにした 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
次も大きなネタバレっぽいです。
重要な修正†
ここまでくると元のコードに1つ明らかなバグがあったことに気づけます。
LANG:java if ( product == null || product.getPrice() >= deposit ) { return createResultWitoutProduct(coinsInserted); }
これだと入金額ぴったりの金額の商品を買えません。
正しくは、
LANG:java if ( product == null || product.getPrice() > deposit ) { return createResultWitoutProduct(coinsInserted); }
なのだと思います。
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
改善されたコード†
あとは、何かあるかなあ・・・
- getValidCoinsDescending の中が格好悪いのだけれど、Java をよく知らないので直せない。
- そもそもコメントの通り定数値を返せるようにすべきだし。
- in.getSelected は Input の文脈では自明なのかもしれないけれど、 このコードの中では in.getSelectedProductId という名前の方がより解りやすい。
- IsProductIdInRange や IsCoinCountInRange で便利に使える Range クラスとかあるならそれを使ってもいい。 この程度のコードなら無理に使わなくても良いけれど。
- calcChange 内のコメントアウトされた throw は適切な例外を投げるべき。いざというときにお客さんに損をさせることになるので念のため。
- createResult(product, coins) という関数を作っても良さそう。ただそれならむしろ new Result(product, coins) で良いはずなのだけれど、そうすると問題としてヒントが少なすぎちゃうから明示的に setProduct, setCoins を呼ぶ形にしたという親心?
とかとか。
in.getSelected() の戻り値†
もしかすると、in.getSelected() の戻り値は producId ではなく brandId なのかもしれないですね。
そうでないと在庫チェックのコードがどこにも見あたらないことになってしまう。
dao にはいろんな種類の商品の在庫が入っていて、押されたボタンに対応する種類の在庫があれば 商品を1つ取り出して返す。product.id は商品毎に異なる値を取り、product.brand.id == in.getSelected() になる。
という解釈が現実的な気がしてきました。
in.getSelected() の戻り値が範囲外であればログに残すのに(システムエラー)、 dao.findById() == null をログに残さないこととも整合します(単なる在庫切れ)。
ただそれならば dao.findById という名前は紛らわしくて、dao.findByBrandId だし、 getPrice は product ではなく brand にあるべきかだし、 あんまり自信が無いので現状はそのままにしておくことにします。
そうじゃなくて†
in.getSelected() の返り値を selectedButtonId あるいは selectedTruckId だと考えるといろいろしっくり来るのかもしれません?
dao.findById() はボタンに対応する在庫があることを確認して、
無ければ null を、
あれば button クラスの実体を返す。
button.getPrice() は問題ないし、
返った Result を処理する部分でも、ボタンに対応する列から商品を1つ出せば良いことになる。
んー、在庫切れのボタンはそもそも「売り切れ」を表示して押せないようにすべきなので、 dao.findById() で null が返るのはやはりシステムエラーにすべき、という話もあるのか・・・
なんで dao.findById() == null に対してはログを取ってないんでしょうね???