Rubyのリファクタリングでイケてないコードをなんとかするやつをやってみた

はじめに

• http://tango-ruby.hatenablog.com/entry/2016/07/15/133122 が面白かったので自分でもやってみた。

リファクタリングできそうな点

• initialize で start_date, end_date を渡しているのが不自然。
  • OrdersReport という汎用的なクラス名なので期間以外の条件で絞り込みたくなった時に応用が効かない
  • クラス名が OrdersDailyReport だったり、仕様として集計時には必ず期間が指定されると決まっていたりしたら気にしなくていいかも
• total_sales_within_date_range で orders の絞り込みと集計を同時にやっている
  • ここも他の条件で絞り込みたくなった時に応用が効かないので絞り込み処理と集計処理を分けたい

リファクタリング結果

変更後

• 呼び出し方は変わって OrdersReport.new(orders).select_within_date_range(start_date, end_date).total_amount となった
• 行数が削減された
• 複数の責務を持つ一つの大きなメソッドから単一の責務を持つ複数の小さなメソッドに分割された
• 絞り込み処理と集計処理をメソッドチェーンで繋げるようにしたので、新しい絞り込み条件が入ってきても拡張しやすくなった

require 'ostruct'

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

  def select_within_date_range(start_date, end_date)
    select { |o| start_date <= o.placed_at && o.placed_at <= end_date }
  end

  def select(&block)
    self.class.new(@orders.select(&block))
  end

  def total_amount
    @orders.map(&:amount).inject(:+)
  end
end

class Order < OpenStruct
end

変更前

require 'ostruct'

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