イケてないコード

以下にあるのがなんかイケてないコード。一応動くし、テストもパスしている。でもそのコード品質は平均よりちょっと下。

範囲を指定してその間の売上の総合計を出すコード。

classOrdersReportdefinitialize(orders, start_date, end_date)@orders = orders@start_date = start_date@end_date = end_dateenddeftotal_sales_within_date_range    orders_within_range = []@orders.eachdo |order|if order.placed_at >=@start_date && order.placed_at <=@end_date        orders_within_range << orderendend    sum =0    orders_within_range.eachdo |order|      sum += order.amountend    sumendendclassOrder <OpenStructend

RSpecテストコードがこれ。

require'spec_helper'describeOrdersReportdo  describe'#total_sales_within_date_range'do    it'returns total sales in range'do      order_within_range1 =Order.new(amount:5,placed_at:Date.new(2016,10,10))      order_within_range2 =Order.new(amount:10,placed_at:Date.new(2016,10,15))      order_out_of_range =Order.new(amount:6,placed_at:Date.new(2016,1,1))      orders = [order_within_range1, order_within_range2, order_out_of_range]      start_date =Date.new(2016,10,1)      end_date =Date.new(2016,10,31)      expect(OrdersReport.             new(orders, start_date, end_date).             total_sales_within_date_range).to eq(15)endendend

ここからリファクタリングを解説していく。
　
　

コードがやってることの解説

コードがやってることの解説。あくまでリファクタリングなので「やってること」は変化しないし、常にテストはパスするべき。

classOrdersReportdefinitialize(orders, start_date, end_date)@orders = orders@start_date = start_date@end_date = end_dateend

まずinitializeでそれぞれの入力値をインスタンス変数に保存する。

deftotal_sales_within_date_range

読んで字のごとく total_sales_within_date_rangeはstart_dateからend_dateの範囲内の売上の合計を返すメソッド。

    orders_within_range = []@orders.eachdo |order|if order.placed_at >=@start_date && order.placed_at <=@end_date        orders_within_range << orderendend

orders_within_rangeという空のArrayを作る。
@ordersをeachでひとつひとつ確認し、そのorderがstart_dateからend_dateの範囲内であればArrayに入れる。
この処理によって orders_within_rangeに範囲内のorderが格納される。

    sum =0    orders_within_range.eachdo |order|      sum += order.amountend    sum

sumに初期値の０を入れる。
orders_within_rangeをeachで回してorderのamountを全てsumに入れて足していく。
sum（売上の合計値）を返す。

classOrder <OpenStructend

OrderはOpenStructで定義されている。OpenStructは手軽に構造体が定義できる便利なクラス。
なのでテストコードなどで以下のようにしてOrderオブジェクトを作ることができる。

order1 =Order.new(amount:5,placed_at:Date.new(2016,10,10))

「イケてない」から「マシ」にするためのリファクタリング

まずRubyに装備されいるステキなやり方がほとんど使われておらず、ひたすらeachで回されている。これを適した方法に変える。そうすることで少ないコード行で実装でき、かつ可読性も上がる。

まずorders_within_rangeに範囲内のorderを格納している処理を変更する。
【変更前】

    orders_within_range = []@orders.eachdo |order|if order.placed_at >=@start_date && order.placed_at <=@end_date        orders_within_range << orderendend

ある条件に合えばそれを取り出す処理はselectで可能になる。
Ruby - selectのドキュメント

上記の処理は次のように置き換えられる。
【変更後】

    orders_within_range =@orders.selectdo |order|      order.placed_at >=@start_date && order.placed_at <=@end_dateend

こうすることで「おおselectか。範囲内のorderを選んで(select)いるんだな」と直感的に理解できる。いちいち頭の中でifの条件があって、、と考えなくてすむ。ちなみにselectの戻り値はArray。
　
次がeachで回して売上合計を出す処理。
【変更前】

    sum =0    orders_within_range.eachdo |order|      sum += order.amountend    sum

sumの宣言などがあってやたらと行数が長い。基本的に１つのメソッドは５行以内にするべき。元のコードは１３行もある。当面はとにかく行数を短縮することだけを考えてリファクタリングしてもいい。ここで使えるのはinject。
Ruby - injectのドキュメント
injectは初期値を設定して中身をぐるぐる回して、それらの結果を返してくれる。
このサイトですごく分かりやすく説明されている。
ruby の inject をわかりやすく説明してみる - Λάδι Βιώσας

injectを使って変更したコードがこれ
【変更後】

    orders_within_range.inject(0)do |sum,order|      sum + order.amountend

injectがsumの初期値である0を設定し、その後ループの中で順次sum + order.amountを実行して総合計を出している。初期値の設定がinjectに入ったことと返り値がsumにしてくれるので行数が減ってスッキリした。
　
次にinjectループの中でorder.amountとかしているのはmapのブロック付きメソッド呼び出しで代用できる。map(&:amount)とかの書き方はある程度Rubyのコードを読んだことがあれば馴染みがあると思う。

mapのブロック付きメソッド呼び出しの詳しい解説はここ。
なぜRubyでブロック付き引数を持つメソッドの引数として&:upcaseみたいな値を渡せるのか | mah365

【変更後】

    orders_within_range.map(&:amount).inject(0)do |sum,amount|      sum + amountend

計算式がsum + amountだけになってスッキリした。
　
これまでの変更点を全て反映した全体像がこれ。
【変更後】

classOrdersReportdefinitialize(orders, start_date, end_date)@orders = orders@start_date = start_date@end_date = end_dateenddeftotal_sales_within_date_range    orders_within_range =@orders.selectdo |order|      order.placed_at >=@start_date && order.placed_at <=@end_dateend    orders_within_range.map(&:amount).inject(0)do |sum,amount|      sum + amountendendendclassOrder <OpenStructend

多少は行数が減って少しだけ可読性も上がったので一応は「マシ」と言えるレベルになったと思う。
とはいってもまだまだそれは「マシ」レベルであって変更されるべき点は多くある。

次回はこれを 2.「マシ」から「いいね」にするためのリファクタリング。
　
　
実はこの３回に渡るブログ記事で書いている内容はほとんど「Practical Object-Oriented Design in Ruby」の受け売り。

Practical Object-Oriented Design in Ruby: An Agile Primer (Addison-Wesley Professional Ruby)

• 作者: Sandi Metz
• 出版社/メーカー: Addison-Wesley Professional
• 発売日: 2012/09/14
• メディア: ペーパーバック
• この商品を含むブログを見る

tango-ruby.hatenablog.com

tango-ruby.hatenablog.com