本日の料理

f:id:nisemono_san:20160722160944j:plain

コードも豚肉も煮こむに限る

はじめに

Rubyのリファクタリングでイケてないコードを美しいオブジェクト指向設計のコードへ改良するための方法 - その１ - Ruby on Railsのビシバシはぁはぁ日記というブログの記事がどうもバズっていて、色んな方面からつっこみが入っていたりする。また、別にこれはオーバーエンジニアリングだから、この程度のコードでもいいのではないか、という指摘も挙がっている。

自分ならどうするかというとこういう感じかな。要するに条件に合う注文を足し算したいだけなので、条件に合う注文を足し算しようよ、という。https://t.co/HEhsO5q2nr
— Rui Ueyama (@rui314) 2016年7月17日

コードを書く人によって「これが良いアプローチだ」というのは千差万別であることは間違いない。元の記事を読む限り、これはどうもRubyの特殊事情なのではないかという疑問が拭えない。そこで、あえて本家オブジェクト指向環境(言語とは言わないマン)であるところのPharo(SmallTalk環境)を利用して、SmallTalkだと、どういう風にリファクタリングできるかを考えてみたい。

「ダメなコード」と言われるものを移植する

まず、元の記事になっているダメなコードを移植する。コードの意図によれば、商品のオーダを、範囲によって絞りこみ、それを追加するという方針を取っている。

ちなみに、一般的な言語のinitialize的なメソッドは、System Browserの中ほどにあるClass sideというものを使って切りかえ、それによってインスタンス生成用のメソッドを用意してあげるのが普通のようだ。画像にすると、下の部分をトグルする。

f:id:nisemono_san:20160723180030p:plain

そして、下のようなメソッドを生成する。

amount: money year: year month: month day: day
    |newOrder|
    newOrder := self new.
    newOrder amount: money;
            place_at: (Date year: year month: month day: day).
    ^newOrder

こうすることによって:

orders := { 
    Order amount: 3000 year: 2016 month: 7 day: 23.
    Order amount: 4000 year: 2016 month: 7 day: 21.
    Order amount: 5000 year: 2016 month: 7 day: 19.
    Order amount: 6000 year: 2016 month: 7 day: 10.
}.

のような、自然な書き方ができるのだが、本体はそっちではなく、このオーダーリストの中から、「7月10日から7月22日」のような間でおこなわれた金額を算出したいということである。ちなみに、元ブログでのイケてないコードは下のようになる:

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

これを愚直に移植すると次のようになる。

orders: orders start_at: start_date end_at: end_date
    |amountSum targetOrders|

    (start_date class = Date) ifFalse: [ self signalInvalidDate ].
    (end_date class   = Date) ifFalse: [ self signalInvalidDate ].
    
    targetOrders := OrderedCollection new.
    orders do: [ :order | 
        ((order place_at > start_date) and: (order place_at < end_date)) 
        ifTrue: [targetOrders add: order]].
    
    amountSum := 0.
    targetOrders do: [ :order | amountSum := (order amount) + amountSum].
    
    ^ amountSum

まず、このコードの何処が駄目だろうか。

前提として、SmallTalkのリテラルでの配列はイミュータブルである。つまり、追加したり、変更したりすることが不可能である。そのため、別途にOrderedCollectionというクラスを使う必要があるのだが、ただこれが出てきたときには、まず「何か変なことをしているんじゃないんだろうか」と疑ったほうがいい可能性がある。

実際に、あとのコードで、条件に当てはまるものを追加するようなコードとなっていて、それをさらに集計しているわけだから二度手間であることは間違いない。

orders: orders start_at: start_date end_at: end_date
    |amountSum targetOrder|

    (start_date class = Date) ifFalse: [ self signalInvalidDate ].
    (end_date class   = Date) ifFalse: [ self signalInvalidDate ].
    
     targetOrder := orders select: [ :order | 
        ((order place_at > start_date) and: (order place_at < end_date))].
    
    amountSum := (targetOrder inject: 0 into: [:sum :order | sum + (order amount)]).    
    ^ amountSum

さて、ここで気になるところが一つある。それは判定式がやたらと長いことだろう。これをもうすこし直感的にするために、メソッドを定義したほうがよいだろう。

元の記事を読むと、このあたりの責務を、OrdersReportの責務にしているわけなんだけれども、これにはちょっと違和感がある。なぜなら、ある時間と時間の範囲に、そのオーダーが属しているかどうかの判断は、オーダーによって判定するほうが筋がいいのではないだろうか。そうすれば、オーダーと、それを集計するクラスとの結合が少なくなる。従って、Orderクラスには次のようなメソッドをはやし:

inRange: start_date end_at: end_date
    ^ ((self place_at > start_date) and: (self place_at < end_date))

OrderCalclatorのメソッドは次のように変更する。

orders: orders start_at: start_date end_at: end_date
    |amountSum targetOrder|

    (start_date class = Date) ifFalse: [ self signalInvalidDate ].
    (end_date class   = Date) ifFalse: [ self signalInvalidDate ].
    
    targetOrder := orders select: [ :order | order inRange: start_date end_at: end_date ].
    
    amountSum := orders inject: 0 into: [:sum :order | sum + (order amount)]).
    ^ amountSum

だいぶ、見通しが良くなってきた。さらに、amountSumの部分に関しても、メソッド化して、簡潔な記述にしておこう。

orders: orders start_at: start_date end_at: end_date
    | targetOrder|

    (start_date class = Date) ifFalse: [ self signalInvalidDate ].
    (end_date class   = Date) ifFalse: [ self signalInvalidDate ].
    
    targetOrder := orders select: [ :order | order inRange: start_date end_at: end_date ].
    ^ self sumOrder: targetOrder

さらに、別に代入する必要もなくなったので

orders: orders start_at: start_date end_at: end_date
    (start_date class = Date) ifFalse: [ self signalInvalidDate ].
    (end_date class   = Date) ifFalse: [ self signalInvalidDate ].
    ^ self sumOrder: (orders select: [ :order | order inRange: start_date end_at: end_date ]).

というわけで、三行になったわけである。めでたしめでたし。

ちょっと違和感のあったところ

さて、元記事ではさらなる提案として、start_atとend_atの引数が必要であるのにも関わらず明示されていないとして、DateRangeというクラスを提唱しているが、自分の目からすればオーバーであるように感じられる。

「これをある程度の規模のプロジェクトと想定し、再利用性」という側面から見ているのだけれども、しかしこの部分だけみればYAGNI原則に引っかかっている印象に見える。また、クラスを挟むことによって、同時に明晰性が失われているように感じる。例えばこのコードの部分を見てみよう:

class OrdersReport
  def initialize(orders, date_range)
    @orders = orders
    @date_range = date_range
  end

  # ...
end

ここに出てくるdate_rangeという変数とは一体なんなのか、全く明晰ではない。これはRubyであるから仕方ないのかもしれない。SmallTalkの場合、次のように書けるので、ある程度、何を目的としたものなのかが、キーワードから推測できるようになっている:

|start_at end_at orders|
orders := { 
    Order amount: 3000 year: 2016 month: 7 day: 22.
    Order amount: 4000 year: 2016 month: 7 day: 21.
    Order amount: 5000 year: 2016 month: 7 day: 19.
    Order amount: 6000 year: 2016 month: 7 day: 10.
}.

start_at := Date year: 2016 month: 7 day: 10.
end_at   := Date year: 2016 month: 7 day: 22.
OrderCalculater orders: orders start_at: start_at  end_at: end_at.