トップ > Ruby > Ruby でイケてないコードを→「いいね」→「スゲーいいね」にリファクタリング？

Ruby でイケてないコードを→「いいね」→「スゲーいいね」にリファクタリング？

Ruby

qiita.com
自分が「Ruby + リファクタリング」でぐぐると上のサイトがいちばん最初にヒットする。内容は題名どおり。添削者はすごい上級者プログラマらしい。添削された人には気の毒なのだけれど、勝手に引用してみる。まず「イケてない」コード。（ごめんなさい）

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, date_range)
    @orders = orders
    @date_range = date_range
  end

  def total_sales_within_date_range
    total_sales(orders_within_range)
  end

  private

  def total_sales(orders)
    orders.map(&:amount).inject(0, :+)
  end

  def orders_within_range
    @orders.select { |order| order.placed_between?(@date_range) }
  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

添削者の言葉。

もう最初のコードと比べると可読性、単一責任、再利用性、疎結合、と全ての要素が満たされて格段に良くなっているのが分かる。ほとんどのメソッド内の行数が１行。（２行なのはinitializeのみ）気持ちいいぐらいに読みやすい。

https://qiita.com/jabba/items/e169adb2f33532c119cf

　
へー、プロの上級者はこんな風にリファクタリングするのだな。でも、自分みたいな初心者素人には謎すぎる。凝りすぎてキモい感じ。「気持ちいいぐらいに読みやすい」ともあまり思えない。まったく自分はセンスがないのだな。「可読性、単一責任、再利用性、疎結合」。確かに、ですな。

初心者素人ならこんな風に書くかも。マネしないように。

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.select {|od| @start_date <= od.placed_at and od.placed_at <= @end_date}
       .map(&:amount).inject(:+)
  end
end

class Order < OpenStruct
end

実質一行。あと、レシーバーが @orders で中身がわかるのに、ブロック変数まで order にするのは長くて自分には見にくい感じ。od で充分に思える。あとは select したら足すだけじゃね？

もちろん DateRange クラスを作ってもいいのだけれど、それは DRY 的に必要になってからでいいと思う。元をシンプルにしておけば改変もラク（select を置き換えればよいのが明白）。「不必要に将来の拡張性を考えない。それが必要になることはたぶんない。」「コードが短くなったら great ! といおう。」

自分に「スゲーいいね」のコードが見にくく感じるのは、置き換えるべき select が一般的な Enumerable なので、（ここでモンキーパッチはダメなので）そのためにいろいろいじらなくてはならないからである。そのために新たにクラスをひとつ追加し、メソッドも大幅に増やしている。これほどのことをするくらいなら、select のままではいけないのだろうか？　プロってのはむずかしいな。


プログラミングにおいてデータ構造は決定的なファクタのひとつだが、この「スゲーいいね」では DateRange クラスというデータ構造を思いついたためにこうなった。結局、この決定がよいかどうかということなのではないか。


※追記（4/2）


植山類さんのこれで充分（そしてそちらの方がたぶんずっと速い）。じつは本文記事は植山さんの「教え」（？）みたいなのが頭にあったのですが、既に本人がお書きになっていました(^^;　自分の場合は Ruby の便利メソッドを使ってメソッドチェーンにしただけで、考え方は植山さんのコードとまったく同じ。植山さんのなら Ruby 以外でもだいたい似たように書けると思う。あと、もしこれだけというのなら OrdersReport クラスがいらないというのもそのとおりですね。