Finder Objectでリファクタしてみた

Daily Blogging91日目

とりあえずcontrollerに全ロジック詰め込んでfat controllerを生成してしまったのでリファクタしてみる。

問題点

今回はcontroller内に下記のロジックが集約されているのが問題

• データ取得
• データのフィルタリング
• レスポンス用のデータの整形

※ブログ用にコードは変更

class CouponsController < ApplicationController
  def index
    @coupons = []

    volume = Volume.current.find_by(id: params[:id])
    return if volume.nil?

    # データ取得
    valid_coupons = UserDiscountCoupon.preload(:discount_coupon).
      where(user_id: user_id).
      valid.
      select("discount_coupon_id, expire_at, count(*) as count").
      group(:discount_coupon_id, :expire_at)

    # データをフィルタリング
    target_coupons = valid_coupons.select do |coupon|
      coupon.usable_coupon?(volume.title)
    end

    # レスポンス用に整形
    @coupons = target_coupons.map do |coupon|
      {
        id: coupon.id,
        name: coupon.name,
        discount_percentage: coupon.discount_percentage,
      }
    end
  end
end

一つのcontrollerで色々な責務が存在しているのであとあと辛くなる
可読性：これだとぱっと見でどんな処理を行なっているのかがわからない
再利用性：データに関するロジックをcontrollerにベタ書きしているので他で再利用できない

責務を分けよう

今回の問題でcontrollerの責務でないのはこの二つ

• データ取得
• データのフィルタリング

この二つはクーポンの責務なので切り離す

Query Object/Finder Object

データの取得に関するロジックを閉じ込めるデザインパターンのこと
Query Objectは聞いたことあったけどFinder Objectは初めて聞いた

二つの違いはこんな感じっぽい

• Query Object
  • 1scopeにつき1 Query Object
  • 複雑なクエリを抽象化する
• Finder Object
  • controllerやserviceクラスから呼び出される
  • ビジネスロジックに基づいたデータ取得を抽象化

どっちを使うのかというよりどっちも使う
クエリ部分はQuery Objectに閉じ込めて、Finder Object内でQuery Objectを呼んだりする

リファクタしてみた

最終的にこんな感じにしてみた

# Query Object
class UserDiscountCoupons::ValidCouponsQuery
  def self.call(user_id)
    UserDiscountCoupon.preload(:discount_coupon).
　　　 where(user_id: user_id).
      valid.
      select("discount_coupon_id, expire_at, count(*) as count").
      group(:discount_coupon_id, :expire_at)
  end
end

# Finder Object
class UserDiscountCoupons::VolumeCouponsFinder
  def self.call(user_id, volume)
    coupons = UserDiscountCoupons::ValidCouponsQuery.call(user_id).select { |coupon| coupon.useable_coupon?(volume.title) }

    coupons.map do |user_discount_coupon|
      {
        id: ~,
        name: ~,
        available_quantity: ~,
        discount_percentage: ~,
        expire_at: ~,
      }
    end
  end
end

class CouponsController < ApplicationController
  def index
    volume = Volume.current.find_by(id: params[:id])
    return if volume.nil?

    @coupons = UserDiscountCoupons::VolumeCouponsFinder.call(auth_token_user.id, volume)
    @coupons.sort_by! { |coupon| coupon[:discount_percentage] }.reverse!
  end
end

データ取得系のロジックを切り離したので、controllerの責務がsortだけになりましたとさ

まとめ

fat controllerを解消するためにとりあえずserviceクラスを採用しがちだったが、
それだと結局serviceクラスに色々な責務が含まれたロジックが移動されるだけなので意味ないよなぁって思ってた。
Query ObjectやFinder Objectならデータの取得に関するロジックの集約と責務がはっきりしているので使いやすそう

今回はFinder Objectでデータ整形しているので、ここは責務を置く場所要検討かも
基本的にこの形でしか使わないのでとりあえずFinder Objectに置いてる