込みいった生成処理をファクトリメソッドに置き換える
元記事はこちら
翻訳
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行が一つの基準になる?
何かで読んだ記憶もある気がする。。
リファクタリングだったか、レガシーコード改善ガイドだったか。。。