(cache) ドヤ顔でリファクタリングされた複雑なコードを、シンプルなオブジェクト指向のコードへ改良するための方法

はてなブックマーク - ドヤ顔でリファクタリングされた複雑なコードを、シンプルなオブジェクト指向のコードへ改良するための方法

オブジェクト指向とデザインパターンが大好きなみなさん、こんにちは。
「オブジェクト指向がしっくりこないstaticおじさん」をバカにしていたら、いつの間にか「モナドが理解できないオブジェクト指向おじさん」と呼ばれるようになってしまったみなさんの胸中、お察しします。
ぜんぶ関数型のせいだ。

さて、「Rubyのリファクタリングでイケてないコードを美しいオブジェクト指向設計のコードへ改良するための方法」という三部作が話題になりました。

その1 (http://tango-ruby.hatenablog.com/entry/2016/07/15/133122)
その2 (http://tango-ruby.hatenablog.com/entry/2016/07/15/133153)
その3 (http://tango-ruby.hatenablog.com/entry/2016/07/15/133217)

しかしリファクタリングされたあとの「美しいオブジェクト指向設計のコード」とやらが、どう見ても無駄に複雑だったので、もっとシンプルなコードに書き換えてみたいと思います。最終的にクラス名もメソッド名も変えるので、リファクタリングではなく書き換えです。

タイトルについて

『Rubyのリファクタリングでイケてないコードを美しいオブジェクト指向設計のコードへ改良するための方法』

・・・などという、長くてわかりにくいタイトルこそ、真っ先にリファクタリングされるべき。「『Rubyのリファクタリングでイケてないコード』ってなんのこっちゃ?」となりませんか?

「イケてないRubyコードを美しいオブジェクト指向設計のコードへリファクタリングする方法」ぐらいでいいでしょう。もし「リファクタリング」を最初に持ってきたいなら「実践リファクタリング：イケてないRubyコードから美しいオブジェクト指向設計のコードへ」とか。

クラスやメソッドの名前について

元記事のコードは、まずクラス名やメソッド名に違和感があります。

扱ってるのが Order なのに、メソッド名が total_sales_xxx() なのはどうなんでしょう。
そもそも金額を合計するのだから、メソッド名は total_amount() のほうがよさそうです。
OrdersReport クラスが、レポートしてるようには見えません。
金額の集計機能がメインなら OrdersAggegation のほうが名前としてはよさそうです。
複数の注文データを扱うことがメイン機能なら、OrdersOperation でよさそうです。
Order クラスの中で使われている placed_at って何? 注文日を表すなら date でいいのでは?
「期間内」を表すメソッドが between? だったり within_range だったり within_date_range だったり、まるで統一感がありません。
「期間」を表すクラスが DateRange ってのはなあ… Period とか Term とか Duration ではだめでしょうか。

ネーミングがダメだと、美しいコードには見えないですね。

OpenStructについて

Ruby の OpenStruct は、書き捨てのスクリプトで使うのはいいけど、そうでないなら使うのは控えたほうがいいと思います。
これを使われると、コードの意図がわからなくなります。

class Order < OpenStruct
  def placed_between?(date_range)
    date_range.include?(placed_at)  # ← この変数はどこから出てきたの?
  end
end

愚直にクラス定義を書きましょう。

class Order
  def initialize(amount, date=nil)  # placed_at を date に変更
    @amount  = amount
    @date    = date || Date.today()
  end
  attr_reader :amount, :date
end

クラス設計について

この OrdersReport クラスでは、「注文データを開始日と終了日で絞り込む」機能と、「注文データの金額を集計する」機能が、ひとつのメソッド total_sales_within_date_range() に含まれています。

けど、この2つの機能は、ひとつのメソッドにする必要はないんじゃないでしょうか。せっかくコンストラクタに開始日と終了日を与えるのだから、その時点で絞り込んでしまえばよさそうです。

class OrdersReport

  ## コンストラクタ (イニシャライザ) に開始日と終了日を指定するなら、
  ## 最初に絞り込めばよい
  def initialize(orders, start_date, end_date)
    @orders = orders.select {|o| start_date <= o.date && o.date <= end_date }
  end

  ## そうすれば、集計機能では日付で絞り込む必要がなく、簡潔になる
  def total_amount
    @orders.map(&:amount).inject(0, :+)
  end

end

それに、コンストラクタで絞り込む必要なんてないです。期間で絞り込む必要がある場合だけそうすればよく、コンストラクタで強制するようなことではありません。次のようなコードで十分です。

## 複数の注文データをまとめて扱うためのクラス
class Orders

  def initialize(orders)
    @orders = orders
  end

  ## 開始日と終了日で絞り込む
  def between(start_date, end_date)
    new_orders = @orders.select {|o| start_date <= o.date && o.date <= end_date }
    return self.class.new(new_orders)
  end

  ## 合計金額を返す
  def total_amount
    return @orders.map(&:amount).inject(0, :+)
  end

end

## 使い方
total = Orders.new(all_orders).between(start_on, end_on).total_amount
puts total

クラス定義の順番について

Order クラスの定義より先に OrdersReport クラスが定義されているの、違和感ありませんか?

DateRangeクラスについて

こんなクラスをわざわざ作らなくても、Ruby なら Range クラスで代用できますよね?

start_on = Date.today()
end_on   = start_on + 7   # 1週間後
p start_on..end_on        # => #<Date: 2016-07-23>..#<Date: 2016-07-30>
p (start_on..end_on).include?(start_on)  #=> true

なお Range クラスのマニュアルによると、『Range#include? は原則として離散値を扱い、 Range#cover? は連続値を扱います』だそうです。日付は離散値なので、covert? より include? のほうがいいでしょう。

「ループよりinject()」について

元記事では、合計値を計算するのにループではなく inject() を使うように進めています。しかし map() や select() は初心者でも理解しやすいですが、inject() はそうではありません¹。

こんなことを書くとまた関数型信者からいろいろ言われるでしょうけど、inject() や reduce() が理解できるだけの能力を持った人は、日本の開発現場ではあまりいません。だいたい、FizzBuzz すら書けない人だらけ²だというのに、なんで inject() が理解できると思うんでしょうかね。大ざっぱにいうと、今仕事で Ruby 書いている人の5割は inject() が書けず、3割は inject() を書いてるけど理解してなくてコピペしてるだけです (もちろん数字は厳密なものではないですが、かなり楽観的な見積もりでこんな感じです)。

なので、合計値を計算するのに必ずしも inject() を使うべきとは思いません³ (使うべきじゃない、ではなく、必ず使えというのは言い過ぎ、という意味)。

また合計値の計算ぐらいなら、ループでも十分簡潔に書けます。ループで簡潔に書けてかつ初心者でも理解できるコードを、格好つけて inject() にする必要はどのくらいありますかね? map() や select() や find() ならともかく。

def total_amount
  ## これで十分見やすいコードになっている
  total = 0; @orders.each {|o| total += o.amount }
  return total
end

まあこんな議論が出てくるのは、Ruby が Enumerable#sum() を用意してくれてないからなんですけどね。Ruby2.4 からは Array#sum() が入るそうですけど、もっと早くに提供してほしかった。しかも Array#sum() であって Enumerable#sum() でないところが、なんともなあ。

テストコードについて

いやー、このテストコードは読みにくいんじゃないかなー。

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

こんなの↓でいいと思います。

require 'spec_helper'

describe OrdersReport do

  describe '#total_sales_within_date_range' do

    it 'returns total sales in range' do
      orders = [
        Order.new(amount:  5, placed_at: Date.new(2016, 10, 10)),
        Order.new(amount: 10, placed_at: Date.new(2016, 10, 15)),
        Order.new(amount:  6, placed_at: Date.new(2016,  1,  1)),  # out of range
      ]
      expected = orders[0].amount + orders[1].amount
      expect(expected).to eq(15)
      #
      start_on = Date.new(2016, 10,  1)
      end_on   = Date.new(2016, 10, 31)
      report = OrdersReport.new(orders, DateRange.new(start_on, end_on))
      expect(report.total_sales_within_date_range).to eq(expected)
    end

  end

end

最終コード

class Order

  def initialize(amount, date=nil)
    @amount  = amount
    @date    = date || Date.today()
  end

  attr_reader :amount, :date

end


class Orders

  def initialize(orders)
    @orders = orders
  end

  def between(start_date, end_date)
    orders = @orders.select {|o| start_date <= o.date && o.date <= end_date }
    return self.class.new(orders)
  end

  def total_amount
    #return @orders.map(&:amount).inject(0, :+)
    total = 0; @orders.each {|o| total += o.amount }
    return total
  end

end

テストコード。RSpec が嫌いなので minitest/ok を使います。これは好みの問題。

require 'date'

require 'minitest/spec'
require 'minitest/autorun'
require 'minitest/ok'

require 'order'


describe Orders do

  describe '#total_amount()' do

    it "returns total amount of orders" do
      orders = [
        Order.new( 5, Date.new(2016, 10, 10)),
        Order.new(10, Date.new(2016, 10, 15)),
        Order.new( 6, Date.new(2016,  1,  1)),  # out of range
      ]
      start_date = Date.new(2016, 10,  1)
      end_date   = Date.new(2016, 10, 31)
      total = Orders.new(orders).between(start_date, end_date).total_amount
      ok {total} == orders[0].amount + orders[1].amount
      ok {total} == 15
    end

  end

end

おわりに

クラス設計を無駄に複雑にするのは、J2EE とその愉快な仲間たちだけにしてほしいです。

私の小岩ホッチキス

カレーの血ライス

ドヤ顔でリファクタリングされた複雑なコードを、シンプルなオブジェクト指向のコードへ改良するための方法

タイトルについて

クラスやメソッドの名前について

OpenStructについて

クラス設計について

クラス定義の順番について

DateRangeクラスについて

「ループよりinject()」について

テストコードについて

最終コード

おわりに

コメントを残すコメントをキャンセル

トラックバックURL

ドヤ顔でリファクタリングされた複雑なコードを、シンプルなオブジェクト指向のコードへ改良するための方法

タイトルについて

クラスやメソッドの名前について

OpenStructについて

クラス設計について

クラス定義の順番について

DateRangeクラスについて

「ループよりinject()」について

テストコードについて

最終コード

おわりに

コメントを残す コメントをキャンセル

トラックバックURL

コメントを残すコメントをキャンセル