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

Ruby のリファクタリングでオブジェクト指向設計に沿った美しいコードになるまでの方法を書いた。

元ネタはこちらの Ben Orenstein 氏のリファクタリングで、そこに私なりの解説とコードを加えた。かなり追加したので Orenstein 氏の原型とはだいぶ違う箇所もあるがオブジェクト指向設計とリファクタリングに対する考え方は同じなはず。

Refactoring from good to great

全３回に渡ってリファクタリングする。

今回は 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 に初期値の０を入れる。
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 の宣言などがあってやたらと行数が長い。基本的に１つのメソッドは５行以内にするべき。元のコードは１３行もある。当面はとにかく行数を短縮することだけを考えてリファクタリングしてもいい。ここで使えるのは 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.「マシ」から「いいね」にするためのリファクタリング。

実はこの３回に渡るブログ記事で書いている内容はほとんど「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」の一択

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

イケてないコード

コードがやってることの解説

「イケてない」から「マシ」にするためのリファクタリング

関連記事