Rubyのリファクタリングでイケてないコードを美しいオブジェクト指向設計のコードへ改良するための方法 - その1
移転しました。
Rubyのリファクタリングでオブジェクト指向設計に沿った美しいコードになるまでの方法を書いた。
元ネタはこちらのBen Orenstein氏のリファクタリングで、そこに私なりの解説とコードを加えた。かなり追加したのでOrenstein氏の原型とはだいぶ違う箇所もあるがオブジェクト指向設計とリファクタリングに対する考え方は同じなはず。
github.com
全3回に渡ってリファクタリングする。
- 「イケてない」から「マシ」にするためのリファクタリング
- 「マシ」から「いいね」にするためのリファクタリング
- 「いいね」から「スゲーいいね」にするためのリファクタリング
今回は1.の「イケてない」から「マシ」にするためのリファクタリング。
イケてないコード
以下にあるのがなんかイケてないコード。一応動くし、テストもパスしている。でもそのコード品質は平均よりちょっと下。
範囲を指定してその間の売上の総合計を出すコード。
class OrdersReport def initialize(orders, start_date, end_date) @orders = orders @start_date = start_date @end_date = end_date end def total_sales_within_date_range orders_within_range = [] @orders.each do |order| if order.placed_at >= @start_date && order.placed_at <= @end_date orders_within_range << order end end sum = 0 orders_within_range.each do |order| sum += order.amount end sum end end class Order < OpenStruct end
RSpecテストコードがこれ。
require 'spec_helper' describe OrdersReport do 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) end end end
ここからリファクタリングを解説していく。
コードがやってることの解説
コードがやってることの解説。あくまでリファクタリングなので「やってること」は変化しないし、常にテストはパスするべき。
class OrdersReport def initialize(orders, start_date, end_date) @orders = orders @start_date = start_date @end_date = end_date end
まずinitializeでそれぞれの入力値をインスタンス変数に保存する。
def total_sales_within_date_range
読んで字のごとく total_sales_within_date_rangeはstart_dateからend_dateの範囲内の売上の合計を返すメソッド。
orders_within_range = [] @orders.each do |order| if order.placed_at >= @start_date && order.placed_at <= @end_date orders_within_range << order end end
orders_within_rangeという空のArrayを作る。
@ordersをeachでひとつひとつ確認し、そのorderがstart_dateからend_dateの範囲内であればArrayに入れる。
この処理によって orders_within_rangeに範囲内のorderが格納される。
sum = 0 orders_within_range.each do |order| sum += order.amount end sum
sumに初期値の0を入れる。
orders_within_rangeをeachで回してorderのamountを全てsumに入れて足していく。
sum(売上の合計値)を返す。
class Order < OpenStruct end
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.each do |order| if order.placed_at >= @start_date && order.placed_at <= @end_date orders_within_range << order end end
ある条件に合えばそれを取り出す処理はselectで可能になる。
Ruby - selectのドキュメント
上記の処理は次のように置き換えられる。
【変更後】
orders_within_range = @orders.select do |order| order.placed_at >= @start_date && order.placed_at <= @end_date end
こうすることで「おおselectか。範囲内のorderを選んで(select)いるんだな」と直感的に理解できる。いちいち頭の中でifの条件があって、、と考えなくてすむ。ちなみにselectの戻り値はArray。
次がeachで回して売上合計を出す処理。
【変更前】
sum = 0 orders_within_range.each do |order| sum += order.amount end sum
sumの宣言などがあってやたらと行数が長い。基本的に1つのメソッドは5行以内にするべき。元のコードは13行もある。当面はとにかく行数を短縮することだけを考えてリファクタリングしてもいい。ここで使えるのはinject。
Ruby - injectのドキュメント
injectは初期値を設定して中身をぐるぐる回して、それらの結果を返してくれる。
このサイトですごく分かりやすく説明されている。
ruby の inject をわかりやすく説明してみる - Λάδι Βιώσας
injectを使って変更したコードがこれ
【変更後】
orders_within_range.inject(0) do |sum, order| sum + order.amount end
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 + amount end
計算式がsum + amountだけになってスッキリした。
これまでの変更点を全て反映した全体像がこれ。
【変更後】
class OrdersReport def initialize(orders, start_date, end_date) @orders = orders @start_date = start_date @end_date = end_date end def total_sales_within_date_range orders_within_range = @orders.select do |order| order.placed_at >= @start_date && order.placed_at <= @end_date end orders_within_range.map(&:amount).inject(0) do |sum, amount| sum + amount end end end class Order < OpenStruct end
多少は行数が減って少しだけ可読性も上がったので一応は「マシ」と言えるレベルになったと思う。
とはいってもまだまだそれは「マシ」レベルであって変更されるべき点は多くある。
次回はこれを 2.「マシ」から「いいね」にするためのリファクタリング。
実はこの3回に渡るブログ記事で書いている内容はほとんど「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