込みいった生成処理をファクトリメソッドに置き換える

元記事はこちら

翻訳

Bad Smell

class InvoicesController < ApplicationController
  def create
    @invoice = Invoice.new(params[:invoice])
    @invoice.address = current_user.address
    @invoice.phone = current_user.phone
    @invoice.vip = (@invoice.amount > 1000)

    if Time.now.day > 15
      @invoice.delivery_time = Time.now + 2.month
    else
      @invoice.delivery_time = Time.now + 1.month
    end

    @invoice.save
  end
end

invoice(請求書)の生成メソッドは込み入っていて、InvoicesControllerが読みづらくなっています。 それに、コントローラーはモデルの生成方法について知りすぎるべきではなく、請求書の生成ロジックはInvoiceモデルに移すべきです。

Refactor

class Invoice < ActiveRecord::Base
  def self.new_by_user(params, user)
    invoice = self.new(params)
    invoice.address = user.address
    invoice.phone = user.phone
    invoice.vip = (invoice.amount > 1000)

    if Time.now.day > 15
      invoice.delivery_time = Time.now + 2.month
    else
      invoice.delivery_time = Time.now + 1.month
    end
  end
end

class InvoicesController < ApplicationController
  def create
    @invoice = Invoice.new_by_user(params[:invoice], current_user)
    @invoice.save
  end
end

new_by_userメソッドをInvoiceモデルに定義し、請求書の作成を担当させます。 こうすることでInvoicesControllerは請求書オブジェクトを得るためにnew_by_userメソッドを呼ぶだけでよくなりました。 “Skinny Controller, Fat Model"原則を忘れないようにしましょう。

感想

最近書いているアクションはほとんど5行以内におさまっていて、非常に読みやすい。 Fat Modelに関しては様々意見があるとは思うけれど、どこまでFatを許容するかは人(プロジェクト)それぞれだと思う。 いくら原則に従っていても、太りすぎたモデルは読みづらいし、何かしらのストレスを感じるほどに太っているコードは リファクタリングするべきというサインじゃないかな。 特にRailsで開発しているのであれば、Modelの肥大化に伴って、テストも肥大化して行くはずだし。 その辺のバランスをどういう風にとっているのか、プラクティスがしりたい。 ここによると100行が一つの基準になる？

何かで読んだ記憶もある気がする。。

リファクタリングだったか、レガシーコード改善ガイドだったか。。。