Rubyのリファクタリングでイケてないコードを美しいオブジェクト指向設計のコードへ改良するための方法 - その２

前回からの続き。
Rubyのリファクタリングでオブジェクト指向設計に沿った美しいコードになるまでの方法を書いた。

1. 「イケてない」から「マシ」にするためのリファクタリング
2. 「マシ」から「いいね」にするためのリファクタリング
3. 「いいね」から「スゲーいいね」にするためのリファクタリング

今回は２.の「マシ」から「いいね」にするためのリファクタリング

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

今回の変更は主にはオブジェクト指向設計の考え方によるリファクタリングになる。
前回までの変更点を反映した全体像

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

まずはorders_within_rangeをprivateメソッドにして外に出してしまうことにする。メソッド名はそのままorders_within_rangeとした。変更後はこのようになる。
【変更後】

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.map(&:amount).inject(0) do |sum, amount|
      sum + amount
    end
  end

  private

  def orders_within_range
    @orders.select do |order|
      order.placed_at >= @start_date && order.placed_at <= @end_date
    end
  end
end

class Order < OpenStruct
end

orders_within_rangeが別メソッドになってprivateになっただけ。カット＆ペーストで一瞬で終わってしまうような変更。ただそれでも意味はおおいにある。理由は３つ。

第一にメソッドの行数が変更前はtotal_sales_within_date_rangeが６行だったのが
変更後はtotal_sales_within_date_rangeが３行、orders_within_rangeが３行になった。ひとつのメソッドに６行あるよりも、２つのメソッドに３行ずつの方がいい。メソッドの行数が長くなるほど可読性が低くなり、単一責任の法則がやぶられてしまう。実際、変更前の６行のtotal_sales_within_date_rangeは単一責任の原則からはみ出ていた。

第二にメソッドを分けることで再利用が可能になる。変更前のぐちゃっと６行ある状態ではどうしても再利用なんてできなかった。メソッドを切り分けることでorders_within_rangeなんかの再利用性が上がった。

第三は可読性。変更後のコードでは読む際に上から順にinitialize、total_sales_within_date_rangeと読んで、その時に「orders_within_rangeはprivateメソッドなのか」と分かる。そこでもし詳しくメソッドの中身を見たければその中を見ればいい。orders_within_rangeという名前から処理内容は明らかで中身をそこまで詳しくみなくてもいい、と考えたならそのままtotal_sales_within_date_rangeを読み続けることができる。
つまりprivateメソッドに切り分けることでそのメソッドは「詳しく見る必要がなければ読み飛ばしてもOk」という意図を示すことができる。

次に変更すべきはprivateメソッドの中
【変更前】

  def orders_within_range
    @orders.select do |order|
      order.placed_at >= @start_date && order.placed_at <= @end_date
    end
  end

ここは以前に本ブログで書いた「聞くな、言え」の法則に反している。
この法則はカンタンに言うとオブジェクトやメソッドにメッセージを渡す時に「このメッセージは渡してもいいでしょうかね？」と聞いてから渡すのではなく、ただ「ほらよ。メッセージだ。受け取れ」と言って渡すだけの方が優れている、という法則。

ここではselectの中でいちいちorder.placed_atが範囲内に入っているかどうかを「聞いている」からよくない。これを「言うだけ」に変更するとこうなる。

【変更後】

  def orders_within_range
    @orders.select do |order|
      order.placed_between?(@start_date, @end_date)
    end
  end

こうすることでただメッセージをorder.placed_between?に投げているだけの処理になる。placed_between?の最後に「？」があるから聞いているように見えるかもしれないが、言いたいことはそこじゃない。条件式を入れて判断せずにメッセージを投げているだけかどうかがポイント。

当然ながらこの変更に伴ってplaced_between?の処理を追加する必要が出てくる。追加した後の全体像がこちら
【変更後の全体像】

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.map(&:amount).inject(0) do |sum, amount|
      sum + amount
    end
  end

  private

  def orders_within_range
    @orders.select do |order|
      order.placed_between?(@start_date, @end_date)
    end
  end
end

class Order < OpenStruct
  def placed_between?(start_date, end_date)
    placed_at >= start_date && placed_at <= end_date
  end
end

これで少し良くなったようだが、新たな問題も出てきた。コード全体を見渡すとあちこちにstart_date, end_dateを引数として使われている。これらのstart_dateとend_dateは毎回入れなければならない。それで可読性と再利用性が下がってしまっている。このコードは再利用するエンジニアに「必ずstart_dateとend_dateで範囲を指定しろよ」と強制しているが明示はしていない。「強制しているが明示はしていない」というのがよくないサイン。

start_date, end_dateというものは常にペアで、どちらかひとつでは成り立たない。なのでそれらをひとまとめにしてしまうのが次にするべき変更。

RubyにはStructというカンタンにデータ構造を定義できるクラスが用意されている。
Ruby - Struckのドキュメント

Structを使えば以下のように範囲指定するクラスが定義できる。名前はDateRangeとした。

class DateRange < Struct.new(:start_date, :end_date)
end

このDateRangeを使うためにまずはテストコードを修正する。
【変更後のテストコード】

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]

      date_range = DateRange.new(Date.new(2016, 10, 1), Date.new(2016, 10, 31))
      expect(OrdersReport.
             new(orders, date_range).
             total_sales_within_date_range).to eq(15)
    end
  end
end

ここでやっているのはそれまでstart_dateとend_dateで指定していた部分をdate_rangeを使うようにしただけ。

次にコードの方も変更する。
【変更後】

class OrdersReport
  def initialize(orders, date_range)
    @orders = orders
    @date_range = date_range
  end

  def total_sales_within_date_range
    orders_within_range.map(&:amount).inject(0) do |sum, amount|
      sum + amount
    end
  end

  private

  def orders_within_range
    @orders.select do |order|
      order.placed_between?(@date_range)
    end
  end
end

class DateRange < Struct.new(:start_date, :end_date)
end

class Order < OpenStruct
  def placed_between?(date_range)
    placed_at >= date_range.start_date && placed_at <= date_range.end_date
  end
end

こうしてDateRangeを使ってひとまとめにすることですごくシンプルにコードが仕上がっている。StructでDataRangeを作るだけで、常にstart_dateとend_dataはペアであるべきということがカンタンに読み取れる。

それだけではない。この変更によって疎結合が達成されている。疎結合とは各コンポーネント間の結合が薄いということ。結合は薄ければ薄いほどいい。
例えばコンポーネントAとコンポーネントBが密接に結合されている状態は変更が容易でなくなる。Aを変更すればBも変更しなければならない、となってしまう。バグが派生するのは往々にして、ある箇所の変更が予想もしなかった別の箇所に影響を与えてしまったことが原因となっている。
もしAとBの間の結合が薄ければAを変更してもBの変更は要らない、となりオブジェクト指向設計で言うところの「容易に変更が可能で再利用もやりやすい」となる。これこそが理想の形。

結合度合いを測るひとつの目安が引数の数。引数の数は少なければ少ないほどいい。引数が多い場合はそれらの変更がたくさん影響を及ぼすことになるからだ。
今回の変更により引数が３つから２つに減らすことができた。

しかしまた新たな問題がある。
Orderクラスの中にplaced_between?があって、その日付の範囲を計算しているがこれはOrderの責任とは言えない。Orderの中に日付範囲を判断する機能がある、ってのは普通の感覚では気付き難い。
これはDateRangeの責任と言える。placed_between?の処理をDateRangeに入れた方が再利用性も高まる。

【変更前】

class Order < OpenStruct
  def placed_between?(date_range)
    placed_at >= date_range.start_date && placed_at <= date_range.end_date
  end
end

日付の範囲の処理をDateRangeに移動した後のコードがこれ。
【変更後】

class DateRange < Struct.new(:start_date, :end_date)
  def include?(date)
    date >= start_date && date <= end_date
  end
end

class Order < OpenStruct
  def placed_between?(date_range)
    date_range.include?(placed_at)
  end
end

これでOrderにとってはただそのdate_rangeにplaced_atが含まれているかどうかを問い合わせればいいだけになった。後はDateRangeがやってくれる。

日付の範囲を判断する処理は単純でdateの大小を２つ比較しているだけ。でもここはもう少しいいRubyの書き方がある。
【変更前】

  def include?(date)
    date >= start_date && date <= end_date
  end

ここはcover?を使うことでシンプルで読みやすい書き方ができる。
Ruby - cover? ドキュメント
【変更後】

  def include?(date)
    (start_date..end_date).cover? date
  end

これは直感的で分かりやすい。Rubyサイコー。

ここまで変更した内容を反映した全体像

class OrdersReport
  def initialize(orders, date_range)
    @orders = orders
    @date_range = date_range
  end

  def total_sales_within_date_range
    orders_within_range.map(&:amount).inject(0) do |sum, amount|
      sum + amount
    end
  end

  private

  def orders_within_range
    @orders.select do |order|
      order.placed_between?(@date_range)
    end
  end
end

class DateRange < Struct.new(:start_date, :end_date)
  def include?(date)
    (start_date..end_date).cover? date
  end
end

class Order < OpenStruct
  def placed_between?(date_range)
    date_range.include?(placed_at)
  end
end

これでだいぶオブジェクト指向設計らしくなってきたので「いいね」ぐらいのレベルになったよう。でもまだまだ「スゲーいいね」にするための余地はある。

次回はこれを 3.「いいね」から「スゲーいいね」にするためのリファクタリング。
　
　
実はこの３回に渡るブログ記事で書いている内容はほとんど「Practical Object-Oriented Design in Ruby」の受け売りで、この本の内容を１００倍ぐらいに薄めて書いたのが本記事。

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

オブジェクト指向に沿ったイケてるコードを書きたい全てのエンジニアにとって「買っても絶対に損は無い」と断言できるオススメの良書。詳しくはこちらの記事に書いた。
tango-ruby.hatenablog.com

tango-ruby.hatenablog.com

tango-ruby.hatenablog.com