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 クラスがいらないというのもそのとおりですね。
