Rubyのリファクタリングでイケてないコードを美しいオブジェクト指向設計のコードへ改良するための方法 - その3
前回からの続き。
Rubyのリファクタリングでオブジェクト指向設計に沿った美しいコードになるまでの方法を書いた。
- 「イケてない」から「マシ」にするためのリファクタリング
- 「マシ」から「いいね」にするためのリファクタリング
- 「いいね」から「スゲーいいね」にするためのリファクタリング
今回は3.の「いいね」から「スゲーいいね」にするためのリファクタリング
「いいね」から「スゲーいいね」にするためのリファクタリング
前回までの変更点を反映した全体像
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
この中でとても読みにくい箇所があって、それはtotal_sales_within_date_rangeの辺り。
【変更前】
def total_sales_within_date_range orders_within_range.map(&:amount).inject(0) do |sum, amount| sum + amount end end
mapしてamountがあって、それをinjectして初期値は0で、、、ってややこしすぎ。
ここはパブリックメソッドであり、クラス外からも参照される箇所になる。そうした外からも参照される箇所は極力シンプルにするようにすべき。誰も複雑なモノを呼びたくない。パブリックメソッドをシンプルで究極に可読性を上げることで、再利用性が高まる。
その下のprivateメソッドは多少複雑でも構わない。
パブリックメソッドをどのぐらいシンプルにするべきか、は「読んだら一瞬で分かるぐらい」がベスト。具体的にはこんな感じ。
【変更後】
def total_sales_within_date_range total_sales(orders_within_range) end
これでもう一目瞭然。total_sales(総売上)はorders_within_range(範囲内のオーダ)から計算する、と。変更前に比べて脳への負荷が1%ぐらいで済む。
この変更にともなってtotal_salesという名のprivateメソッドに処理を移行させる。
private def total_sales(orders) orders.map(&:amount).inject(0) do |sum, amount| sum + amount end end
ここにまだ一工夫ができる。
injectのちょっとした用法で:+を使うことで1行で表現できる。慣れている人にとってはお馴染みの方法で可読性がさらに上がる。
【変更後】
def total_sales(orders) orders.map(&:amount).inject(0, :+) end
なんとたったの1行で表現できる。:+の詳しい解説はこことか分かりやすい。
ちょっとわかりにくいけど非常に便利なinjectメソッド - 勉強日記
もう:+はそのまま覚えておいても損はない。
これでリファクタリングは終了。これまでの全てのリファクタリングを反映させた全体像がこれ。
【完成したコード】
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
もう最初のコードと比べると可読性、単一責任、再利用性、疎結合、と全ての要素が満たされて格段に良くなっているのが分かる。ほとんどのメソッド内の行数が1行。(2行なのはinitializeのみ)気持ちいいぐらいに読みやすい。
【リファクタリング前のイケてないコード】
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
最初のイケてなかった頃の変更前と変更後とを比較した。
変更前 | 変更後 | |
---|---|---|
可読性 | 読めても時間かかる | スッキリ読める |
責任 | 1メソッドに複数責任 | キレイに単一責任 |
再利用性 | ほぼ無理 | 容易に再利用可能 |
結合 | 密結合 | 疎結合 |
要素毎に再確認すると凄まじいぐらいに改善されていることが分かる。
今回の例にしたコードだけを見ると「1回しか呼んでない引数にわざわざStructなんか定義して、それ必要か?」とか思われそうだが、それはこのコード例だけを見ているからだ。これがある程度の規模のプロジェクトの一部だと仮定すると、より分かりやすくなる。その規模になれば必ず誰かがどこかで再利用する。その際に変更前と変更後のコードと、どちらがよりプロジェクトにとって、メンテナンスが容易で再利用がしやすいか?と考えれば明らかに変更後の方になる。
「期間内の総売上を出力するコードを書いてくれ」と頼まれて、そのままそれだけを書いてしまったのが変更前のコード。
「期間内の総売上の出力が必要でかつ、後で誰かがこのコードをメンテナンスして、また一部を再利用することがあるな」と想定して作られたのが変更後のコードになる。誰でも後者の方の考えを持つエンジニアと一緒に働きたいはず。
ということでこれで「Rubyのリファクタリングでイケてないコードを美しいオブジェクト指向設計へ改良するための全解説」は終わり。
実はここに書いている内容はほとんど「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
- メディア: ペーパーバック
- この商品を含むブログを見る
tango-ruby.hatenablog.com
Rubyのリファクタリングでイケてないコードを美しいオブジェクト指向設計のコードへ改良するための方法 - その2
前回からの続き。
Rubyのリファクタリングでオブジェクト指向設計に沿った美しいコードになるまでの方法を書いた。
- 「イケてない」から「マシ」にするためのリファクタリング
- 「マシ」から「いいね」にするためのリファクタリング
- 「いいね」から「スゲーいいね」にするためのリファクタリング
今回は2.の「マシ」から「いいね」にするためのリファクタリング
「マシ」から「いいね」にするためのリファクタリング
今回の変更は主にはオブジェクト指向設計の考え方によるリファクタリングになる。
前回までの変更点を反映した全体像
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になっただけ。カット&ペーストで一瞬で終わってしまうような変更。ただそれでも意味はおおいにある。理由は3つ。
第一にメソッドの行数が変更前はtotal_sales_within_date_rangeが6行だったのが
変更後はtotal_sales_within_date_rangeが3行、orders_within_rangeが3行になった。ひとつのメソッドに6行あるよりも、2つのメソッドに3行ずつの方がいい。メソッドの行数が長くなるほど可読性が低くなり、単一責任の法則がやぶられてしまう。実際、変更前の6行のtotal_sales_within_date_rangeは単一責任の原則からはみ出ていた。
第二にメソッドを分けることで再利用が可能になる。変更前のぐちゃっと6行ある状態ではどうしても再利用なんてできなかった。メソッドを切り分けることで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の変更は要らない、となりオブジェクト指向設計で言うところの「容易に変更が可能で再利用もやりやすい」となる。これこそが理想の形。
結合度合いを測るひとつの目安が引数の数。引数の数は少なければ少ないほどいい。引数が多い場合はそれらの変更がたくさん影響を及ぼすことになるからだ。
今回の変更により引数が3つから2つに減らすことができた。
しかしまた新たな問題がある。
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の大小を2つ比較しているだけ。でもここはもう少しいい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.「いいね」から「スゲーいいね」にするためのリファクタリング。
実はこの3回に渡るブログ記事で書いている内容はほとんど「Practical Object-Oriented Design in Ruby」の受け売りで、この本の内容を100倍ぐらいに薄めて書いたのが本記事。
Practical Object-Oriented Design in Ruby: An Agile Primer (Addison-Wesley Professional Ruby)
- 作者: Sandi Metz
- 出版社/メーカー: Addison-Wesley Professional
- 発売日: 2012/09/14
- メディア: ペーパーバック
- この商品を含むブログを見る
tango-ruby.hatenablog.com
Rubyのリファクタリングでイケてないコードを美しいオブジェクト指向設計のコードへ改良するための方法 - その1
Rubyのリファクタリングでオブジェクト指向設計に沿った美しいコードになるまでの方法を書いた。
元ネタはこちらのBen Orenstein氏のリファクタリングで、そこに私なりの解説とコードを加えた。かなり追加したのでOrenstein氏の原型とはだいぶ違う箇所もあるがオブジェクト指向設計とリファクタリングに対する考え方は同じなはず。
github.com
全3回に渡ってリファクタリングする。
- 「イケてない」から「マシ」にするためのリファクタリング
- 「マシ」から「いいね」にするためのリファクタリング
- 「いいね」から「スゲーいいね」にするためのリファクタリング
今回は1.の「イケてない」から「マシ」にするためのリファクタリング。
イケてないコード
以下にあるのがなんかイケてないコード。一応動くし、テストもパスしている。でもそのコード品質は平均よりちょっと下。
範囲を指定してその間の売上の総合計を出すコード。
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, start_date, end_date) @orders = orders @start_date = start_date @end_date = end_date end
まずinitializeでそれぞれの入力値をインスタンス変数に保存する。
def total_sales_within_date_range
読んで字のごとく total_sales_within_date_rangeはstart_dateからend_dateの範囲内の売上の合計を返すメソッド。
orders_within_range = [] @orders.each do |order| if order.placed_at >= @start_date && order.placed_at <= @end_date orders_within_range << order end end
orders_within_rangeという空のArrayを作る。
@ordersをeachでひとつひとつ確認し、そのorderがstart_dateからend_dateの範囲内であればArrayに入れる。
この処理によって orders_within_rangeに範囲内のorderが格納される。
sum = 0 orders_within_range.each do |order| sum += order.amount end sum
sumに初期値の0を入れる。
orders_within_rangeをeachで回してorderのamountを全てsumに入れて足していく。
sum(売上の合計値)を返す。
class Order < OpenStruct end
OrderはOpenStructで定義されている。OpenStructは手軽に構造体が定義できる便利なクラス。
なのでテストコードなどで以下のようにしてOrderオブジェクトを作ることができる。
order1 = Order.new(amount: 5, placed_at: Date.new(2016, 10, 10))
「イケてない」から「マシ」にするためのリファクタリング
まずRubyに装備されいるステキなやり方がほとんど使われておらず、ひたすらeachで回されている。これを適した方法に変える。そうすることで少ないコード行で実装でき、かつ可読性も上がる。
まずorders_within_rangeに範囲内のorderを格納している処理を変更する。
【変更前】
orders_within_range = [] @orders.each do |order| if order.placed_at >= @start_date && order.placed_at <= @end_date orders_within_range << order end end
ある条件に合えばそれを取り出す処理はselectで可能になる。
Ruby - selectのドキュメント
上記の処理は次のように置き換えられる。
【変更後】
orders_within_range = @orders.select do |order| order.placed_at >= @start_date && order.placed_at <= @end_date end
こうすることで「おおselectか。範囲内のorderを選んで(select)いるんだな」と直感的に理解できる。いちいち頭の中でifの条件があって、、と考えなくてすむ。ちなみにselectの戻り値はArray。
次がeachで回して売上合計を出す処理。
【変更前】
sum = 0 orders_within_range.each do |order| sum += order.amount end sum
sumの宣言などがあってやたらと行数が長い。基本的に1つのメソッドは5行以内にするべき。元のコードは13行もある。当面はとにかく行数を短縮することだけを考えてリファクタリングしてもいい。ここで使えるのはinject。
Ruby - injectのドキュメント
injectは初期値を設定して中身をぐるぐる回して、それらの結果を返してくれる。
このサイトですごく分かりやすく説明されている。
ruby の inject をわかりやすく説明してみる - Λάδι Βιώσας
injectを使って変更したコードがこれ
【変更後】
orders_within_range.inject(0) do |sum, order| sum + order.amount end
injectがsumの初期値である0を設定し、その後ループの中で順次sum + order.amountを実行して総合計を出している。初期値の設定がinjectに入ったことと返り値がsumにしてくれるので行数が減ってスッキリした。
次にinjectループの中でorder.amountとかしているのはmapのブロック付きメソッド呼び出しで代用できる。map(&:amount)とかの書き方はある程度Rubyのコードを読んだことがあれば馴染みがあると思う。
mapのブロック付きメソッド呼び出しの詳しい解説はここ。
なぜRubyでブロック付き引数を持つメソッドの引数として&:upcaseみたいな値を渡せるのか | mah365
【変更後】
orders_within_range.map(&:amount).inject(0) do |sum, amount| sum + amount end
計算式がsum + amountだけになってスッキリした。
これまでの変更点を全て反映した全体像がこれ。
【変更後】
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
多少は行数が減って少しだけ可読性も上がったので一応は「マシ」と言えるレベルになったと思う。
とはいってもまだまだそれは「マシ」レベルであって変更されるべき点は多くある。
次回はこれを 2.「マシ」から「いいね」にするためのリファクタリング。
実はこの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
- メディア: ペーパーバック
- この商品を含むブログを見る
tango-ruby.hatenablog.com