- Published on
Rubyのリファクタリングでイケてないコードを美しいオブジェクト指向設計のコードへ改良するための方法 - その２
- Authors
- ジャバ・ザ・ハットリ
前回からの続き。
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.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) |
| 作者: Sandi Metz |
| 出版社/メーカー: Addison-Wesley Professional |
| 発売日: 2012/09/14 |
| メディア: ペーパーバック |
オブジェクト指向に沿ったイケてるコードを書きたい全てのエンジニアにとって「買っても絶対に損は無い」と断言できるオススメの良書。詳しくはこちらの記事に書いた。
英語とプログラミングを同時に勉強するなら「Practical Object-Oriented Design in Ruby」の一択