Zenn
🐱

Rubyでリファクタリングをやってみよう

2020/12/03に公開
3件
Ruby
初心者
リファクタリング
tech

これは「フィヨルドブートキャンプ Part 1 Advent Calendar 2020」の3日目の記事です。
https://adventar.org/calendars/5086/

昨日はべこさんの
日報に書いたイラストたち #fjordbootcamp - 仕事が趣味です
でした！
イラストがとっても可愛いかつ、わかりやすいです！

Part2 もあります。
フィヨルドブートキャンプ Part 2 Advent Calendar 2020 - Adventar

フィヨルドブートキャンプのプラクティスの進捗も終盤に差しかかってきたので、本記事では学習を振り返って一番楽しかったRubyでリファクタリングする話を書きます。

リファクタリングとは

そもそもリファクタリングとは何か？ですが、リファクタリング 既存のコードを安全に改善する（第2版）には以下のように定義されています。

外部から見たときの振る舞いを保ちつつ、理解や修正が簡単になるように、ソフトウェアの内部構造を変化させること

つまり、機能が変わるような変更はリファクタリングとは呼びません。
ユーザーから見てシステムの振る舞いが変わるからです。

じゃあ具体的にリファクタリングって何をすればいいのかというと、それを学ぶ手法は大きく分けて以下の2つになると思っています（あくまで個人の考えなので他にもあるかも）。

  • 公式リファレンスなどを読んでこんな書き方できそうと自分で考えてみる
  • ブログや技術書などから先人たちが過去に実践しているリファクタリングの手法を知り、それを真似してみる

リファクタリングのやり方を知って既存のコードに適用し、コードの構造が改善されることによって他の人にとっても理解しやすく変更に強いコードになります。
もちろんリファクタリングを行う本人にとっても、カオスなコードをリファクタリングしてスッキリした瞬間はとても快感なのではと思います。

この記事では私がフィヨルドブートキャンプで学習し、活用したリファクタリング手法の一部を紹介します。
これを読んで少しでもリファクタリングやってみたいと思う方が増えたら幸いです。

いざ、リファクタリング

では、早速以下のコードをリファクタリングしていきます。
これは指定した期間の映画チケットの売上を算出するサンプルプログラムです。
Purchaserクラスでチケット購入者の情報、Salesクラスでチケットの売上を管理しています。
Salesクラスのcalcメソッドで指定した期間の売上を算出していますが、このメソッドでリファクタリングできそうな箇所がいくつかあります。

class Purchaser
  attr_reader :section, :purchased_at

  def initialize(section:, purchased_at:)
    @section = section # 購入者の区分（こども、おとな、シニア）
    @purchased_at = purchased_at
  end
end

class Sales
  def initialize(purchasers, start_date, end_date)
    @purchasers = purchasers
    @start_date = start_date
    @end_date = end_date
  end

  def calc
    # start_dateからend_dateの期間内に購入されたデータを集める
    target_purchasers = []
    @purchasers.each do |purchaser|
      target_purchasers << purchaser if purchaser.purchased_at >= @start_date && purchaser.purchased_at <= @end_date
    end

    # 集めたデータの購入金額の合計を算出する
    sales = 0
    target_purchasers.each do |purchaser|
      # 購入者の区分（こども、おとな、シニア）によって購入金額を決定する
      sales += case purchaser.section
      when :child
        1000
      when :adult
        1800
      when :senior
        1100
      end
    end

    sales
  end
end

テストコードは以下のとおりです。
リファクタリングしても振る舞いが変わらないことを保証するためには、リファクタリングする度にテストが通ることを確認するのが良さそうです。
人の目で確認できなくはないのですが確認漏れが発生する可能性があるので、テストコードがあったほうが安心してリファクタリングに取り組むことができます。

require "date"

class SalesTest < Minitest::Test
  def test_calc_sales
    # 11月1日から11月30日までの売上を算出するテスト
    purchasers = []
    purchasers << Purchaser.new(section: :child, purchased_at: Date.new(2020, 11, 16))
    purchasers << Purchaser.new(section: :adult, purchased_at: Date.new(2020, 11, 24))
    purchasers << Purchaser.new(section: :senior, purchased_at: Date.new(2020, 11, 30))
    purchasers << Purchaser.new(section: :adult, purchased_at: Date.new(2020, 11, 9))
    # 以下のデータだけ期間外なので集計されない
    purchasers << Purchaser.new(section: :adult, purchased_at: Date.new(2020, 10, 31))

    sales = Sales.new(purchasers, Date.new(2020, 11, 1), Date.new(2020, 11, 30)).calc
    assert_equal 5700, sales
  end
end

Rubyの便利メソッドを活用する

Rubyには配列の操作に関する様々な便利メソッドが揃っており、その使い方を理解することでできるリファクタリング手法があります。

例えばcalcメソッドの以下のコードではtarget_purchasersという配列を用意してから、ifで指定した条件に一致するデータをこの配列に入れています。

class Sales
# ...
  def calc
    target_purchasers = []
    @purchasers.each do |purchaser|
      # 指定された期間内に購入されたデータをtarget_purchasersに保存
      target_purchasers << purchaser if purchaser.purchased_at >= @start_date && purchaser.purchased_at <= @end_date
    end
    # ...
  end

target_purchasers = []とわざわざ書かなくてもeachの代わりにmapを使えば以下のように書くことができます。
mapはブロックで評価した値すべてを配列で返すメソッドです。

def calc
  target_purchasers = @purchasers.map do |purchaser|
    purchaser if purchaser.purchased_at >= @start_date && purchaser.purchased_at <= @end_date
  end
  # ...
end

mapはブロックで評価した値をそのまま返すので、if文の条件に一致しない場合はnilを返します。
今回はif文の条件に一致したデータだけが欲しいので、Ruby2.7で追加されたfilter_mapメソッドを使います。
filter_mapメソッドはブロックで評価した値のうち、真になった値を返すメソッドです。
（2020/12/10追記）filter_mapメソッドを使っていましたが、この場合はブロックの中でpurchaserに対する操作を行わず真になった値をそのまま返すので、selectを使ったほうが良さそうです。
selectの公式リファレンス
filter_mapの公式リファレンス

def calc
  target_purchasers = @purchasers.select do |purchaser|
    purchaser.purchased_at >= @start_date && purchaser.purchased_at <= @end_date
  end
  # ...
end

さらに、if文の日付範囲内かどうかの確認はcover?メソッドを使うともっと簡潔に書くことができます。

def calc
  target_purchasers = @purchasers.select do |purchaser|
    (@start_date..@end_date).cover?(purchaser.purchased_at)
  end
  # ...
end

ちなみにRubyやRailsでリファクタリングに使えそうな便利メソッドについては、フィヨルドブートキャンプのメンターの伊藤さんも以下の記事で解説されています。
[初心者向け] RubyやRailsでリファクタリングに使えそうなイディオムとか便利メソッドとか

eachではなくmapなどのコレクション系メソッドを使うという点に関してはこちらの記事で詳しく解説されています。
Ruby: eachよりもmapなどのコレクションを積極的に使おう（社内勉強会）｜TechRacho（テックラッチョ）〜エンジニアの「？」を「！」に〜｜BPS株式会社

変更後のコードは変更前に比べて読みやすくはなっています。しかし、calcメソッドにはまだまだリファクタリングできそうな箇所があります。

変更前

def calc
  target_purchasers = []
  @purchasers.each do |purchaser|
    target_purchasers << purchaser if purchaser.purchased_at >= @start_date && purchaser.purchased_at <= @end_date
  end
  # ...
end

変更後

def calc
  target_purchasers = @purchasers.select do |purchaser|
    (@start_date..@end_date).cover?(purchaser.purchased_at)
  end
  # ...
end

1メソッドに与える仕事は１つにする

改めてcalcメソッド全体を眺めてみると行数が20行近くあります。
このメソッドは、「指定した日付範囲のデータを集める」、「集めたデータの売上を算出する」という二つの仕事をしており、仮に「指定した日付範囲のデータを集める」処理だけ別の箇所で使いたいとなっても再利用することができません。
また、メソッドが長ければ長いほど処理の流れを一から読んでいかなければならなくなるので、コードを追うのがつらくなります。

def calc
  # データを集める
  target_purchasers = @purchasers.select do |purchaser|
    purchaser.purchased_at >= @start_date && purchaser.purchased_at <= @end_date
  end

  # 売上を算出する
  sales = 0
  target_purchasers.each do |purchaser|
    sales += case purchaser.section
    when :child
      1000
    when :adult
      1800
    when :senior
      1100
    end
  end

  sales
end

そうならないために、データを集める処理をprivateメソッドに切り出します。

def calc
  sales = 0
  target_purchasers.each do |purchaser|
    sales += case purchaser.section
    when :child
      1000
    when :adult
      1800
    when :senior
      1100
    end
  end

  sales
end

private
  def target_purchasers
    @purchasers.select do |purchaser|
      (@start_date..@end_date).cover?(purchaser.purchased_at)
    end
  end

それから、target_purchasersという名前が指すものが曖昧なので、メソッド名をpurchasers_within_dateに変更します。
こうすればpurchasers_within_dateメソッドの中身を一から追わなくても、日付範囲内の購入者データを取得するメソッドであることがメソッド名から推測できるようになります。

def calc
  sales = 0
  purchasers_within_date.each do |purchaser|
    sales += case purchaser.section
    when :child
      1000
    when :adult
      1800
    when :senior
      1100
    end
  end

  sales
end

private
  def purchasers_within_date
    @purchasers.select do |purchaser|
      (@start_date..@end_date).cover?(purchaser.purchased_at)
    end
  end

メソッドを切り出したことで変更前のコードに比べてcalcメソッドがスリムになってきました。
ただ、まだ責務を切り分けられる点がありそうです。

データのまとまりをStructに切り出す

このcase文を使っているところが少し冗長な感じがします。いまは３つしか区分が無いので良いのですが、今後区分が増えていくとcase文がどんどん肥大化していきます。

def calc
  sales = 0
  purchasers_within_date.each do |purchaser|
    sales += case purchaser.section
    when :child
      1000
    when :adult
      1800
    when :senior
      1100
    end
  end
  
  sales
end

RubyにはStructという構造体のクラスがあり、これを使うことで意味のあるデータのまとまりを定義することができます。
case文のsection(:childや:adult)と金額の組み合わせはデータのまとまりなので、これをStructで書き換えてみます。

# 区分（こども、おとな、シニア）と金額のデータ構造をStructで定義する
Section = Struct.new(:sym, :charge)
SECTION = [
  Section.new(:child, 1000),
  Section.new(:adult, 1800),
  Section.new(:senior, 1100)
].each_with_object({}){ |section, h| h[section.sym] = section }.freeze

class Purchaser
  attr_reader :section, :purchased_at

  def initialize(section:, purchased_at:)
    # 上で定義したStructをPurchaserクラスのインスタンス変数に代入する
    # SECTIONの中身は以下のとおり
    # => {:child=>#<struct Section sym=:child, charge=1000>, :adult=>#<struct Section sym=:adult, charge=1800>, :senior=>#<struct Section sym=:senior, charge=1100>}
    # たとえばinitializeの引数のsectionに:childが指定されると、#<struct Section sym=:child, charge=1000>が@sectionに代入される
    @section = SECTION[section]
    @purchased_at = purchased_at
  end
end

Purchaserクラスのインスタンス変数@sectionにStructを代入したので、Salesクラスのcase文を削除し、sales += purchaser.section.chargeに書き換えます。
purchaser.section.chargeと書くことで、購入者の区分ごとの金額を表していることが伝わりやすくなりました。

class Sales
# ...
  def calc
    sales = 0
    purchasers_within_date.each do |purchaser|
      sales += purchaser.section.charge
    end
  
    sales
  end
# ...

最後に、calcメソッドのeachをinjectに書き換えると、最初２０行近くあったcalcメソッドを1行まで減らすことができました。

def calc
  purchasers_within_date.inject(0) { |sum, purchaser| sum + purchaser.section.charge }
end

これでリファクタリングは完了です。
改めて変更前と変更後のコードを比較してみると、かなり見通しが良くなっていることがわかります。
行数的にはほぼ同じなのですが、変更後のほうがコードの意図が伝わりやすいです。

変更前

class Purchaser
  attr_reader :section, :purchased_at

  def initialize(section:, purchased_at:)
    @section = section
    @purchased_at = purchased_at
  end
end

class Sales
  def initialize(purchasers, start_date, end_date)
    @purchasers = purchasers
    @start_date = start_date
    @end_date = end_date
  end

  def calc
    target_purchasers = []
    @purchasers.each do |purchaser|
      target_purchasers << purchaser if purchaser.purchased_at >= @start_date && purchaser.purchased_at <= @end_date
    end

    sales = 0
    target_purchasers.each do |purchaser|
      sales += case purchaser.section
      when :child
        1000
      when :adult
        1800
      when :senior
        1100
      end
    end

    sales
  end
end

変更後

Section = Struct.new(:sym, :charge)
SECTION = [
  Section.new(:child, 1000),
  Section.new(:adult, 1800),
  Section.new(:senior, 1100)
].each_with_object({}){ |section, h| h[section.sym] = section }.freeze

class Purchaser
  attr_reader :section, :purchased_at

  def initialize(section:, purchased_at:)
    @section = SECTION[section]
    @purchased_at = purchased_at
  end
end

class Sales
  def initialize(purchasers, start_date, end_date)
    @purchasers = purchasers
    @start_date = start_date
    @end_date = end_date
  end

  def calc
    purchasers_within_date.inject(0) { |sum, purchaser| sum + purchaser.section.charge }
  end
  
  private
    def purchasers_within_date
      @purchasers.select do |purchaser|
        (@start_date..@end_date).cover?(purchaser.purchased_at)
      end
    end
end

まとめ

上で紹介した方法はあくまでリファクタリングの手法の一つなので、もっと良い方法があるかもしれません。
自分だったらこうリファクタリングする！という方は是非トライしてみてください。
この記事を見て自分が過去に書いたコードを改善できそうというものがあれば是非リファクタリングしてみましょう！
この記事を書く過程でテストコードに何度も助けられたので、リファクタリングする際はテストコードをお供にすると心強いです。
Let's Enjoy リファクタリング！

参考

Discussion

yharayhara

今回はif文の条件に一致したデータだけが欲しいので、Ruby2.7で追加されたfilter_mapメソッドを使います。

この場合はselectメソッドを使うといいですよ :-)

hogucchogucc

@yhara
コメントありがとうございます！
おっしゃるとおりですね💦本文に追記してfilter_mapをselectに修正しました！

石谷太一石谷太一

自分なら、#calc は、

  • Purchaser#charge を追加
  • Array#sum を使用

で、以下のように書きますね。

class Purchaser
  # 追加
  def charge
    @section.charge
  end
end

class Sales
  def calc
    # #inject ではなく #sum を使用
    purchasers_within_date.sum(&:charge)
  end
end