モデルのコールバックを使う
元記事はこちら
翻訳
Bad Smell
<% form_for @post do |f| %> <%= f.text_field :content %> <%= check_box_tag 'auto_tagging' %> <% end %> class PostsController < ApplicationController def create @post = Post.new(params[:post]) if params[:auto_tagging] == '1' @post.tags = AsiaSearch.generate_tags(@post.content) else @post.tags = "" end @post.save end end
上記の例では、ユーザーがauto_taggingチェックボックスをクリックしていた場合に、postの内容に従ってtagsを生成し、保存前にpostにtagsの値をセットします。モデルのコールバックを使えば、tagsの値をセットする処理をモデルに移動できます。
Refactor
class Post < ActiveRecord::Base attr_accessor :auto_tagging before_save :generate_tagging private def generate_taggings return unless auto_tagging == '1' self.tags = Asia.search(self.content) end end <% form_for @post do |f| %> <%= f.text_field :content %> <%= f.check_box :auto_tagging %> <% end %> class PostsController < ApplicationController def create @post = Post.new(params[:post]) @post.save end end
上記の通り、before_saveコールバックで、ユーザーがauto_taggingをクリックしていた場合にtagsを生成して、値をセットするPostモデルのgenerate_taggingを作成しました。これでPostsControllerでモデルロジックを気にする必要がなくなりました。
感想
before_save
うまく使えるとコントローラーのロジックをぐっと減らせるので便利。
込みいった生成処理をファクトリメソッドに置き換える
元記事はこちら
翻訳
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行が一つの基準になる?
何かで読んだ記憶もある気がする。。
リファクタリングだったか、レガシーコード改善ガイドだったか。。。
モデルのロジックをモデルに移動する
元記事はこちら
翻訳
Bad Smell
class PostsController < ApplicationController def publish @post = Post.find(params[:id]) @post.update_attribute(:is_published, true) @post.approved_by = current_user if @post.created_at > Time.now - 7.days @post.popular = 100 else @post.popular = 0 end redirect_to post_url(@post) end end
上記の例では、PostsControllerはpostを公開したいので、 まずpostを検索し、 postのis_published属性とapproved*1に値をセットし、 popular属性にcreated_atの値をもとに値を設定してと、 PostsControllerはロジックを知りすぎており、コントローラーの責務ではなく、モデルが操作するべきです。
Refactor
class Post < ActiveRecord::Base def publish self.is_published = true self.approved_by = current_user if self.created_at > Time.now - 7.days self.popular = 100 else self.popular = 0 end end end class PostsController < ApplicationController def publish @post = Post.find(params[:id]) @post.publish redirect_to post_url(@post) end end
公開ロジックをコントローラーからモデルに移動してpublishメソッドをPostモデルを作ったので、コントローラーからpublishメソッドを呼ぶだけでよくなりました。 これで綺麗になりました。
感想
モデル内でcurrent_userを使っているのが気になるなぁとおもった。
*1:approved_byの誤り?
モデルに仮想的な属性を追加する
元記事はこちら
翻訳
Bad Smell
<% form_for @user do |f| %> <%= text_field_tag :full_name %> <% end %> class UsersController < ApplicationController def create @user = User.new(params[:user]) @user.first_name = params([:full_name]).split(' ', 2).first @user.last_name = params([:full_name]).split(' ', 2).last @user.save end end
上記の例では、full_name入力欄がある画面ですが、データベースには姓、名を分けて保存したいので、コントローラーで姓と名を分割して@userオブジェクトに設定しています。 しかし、モデルに属性を設定するのは、コントローラーの責務ではありませんから、仮想的な属性により、この処理をモデルにさせるべきです。
Refactor
class User < ActiveRecord::Base def full_name [first_name, last_name].join(' ') end def full_name=(name) split = name.split(' ', 2) self.first_name = split.first self.last_name = split.last end end <% form_for @user do |f| %> <%= f.text_field :full_name %> <% end %> class UsersController < ApplicationController def create @user = User.create(params[:user]) end end
こうすることで、Userモデルにfull_name=
という仮想的な属性が付加され、姓と名を設定するするようになりました。
こうすれば、たった1行でuserを作成することができるようになり、Userモデルの中で姓と名を分割できるようになるので、いい感じになります。
感想
分割して入力された電話番号をつなげてDBに放り込みたいとか、例とは逆の方向でなら使う機会は多そう。
Railsガイドによると、confirmation: true
な時にも仮想属性が生まれてるみたい。
スコープを使ってアクセスする
元記事はこちら
翻訳
オブジェクトのオーナーとcurrent_userを比較することで、アクセス権をチェックしたいとおもったら、そんな冗長でイケてない書き方はせずに、スコープをつかてアクセスしましょう。
Bad Smell
class PostsController < ApplicationController def edit @post = Post.find(params[:id]) if @post.user != current_user flash[:warning] = 'Access denied' redirect_to posts_url end end end
上記の例では、postのuserをcurrent_userと比較して、一致しなかった場合に、postの編集を許可しないようにしている。 しかし、このやり方は、edit,update,destroyなどを実行できるかチェックするには冗長すぎる。スコープを使ってリファクタリングしよう。
Refactor
class PostsController < ApplicationController def edit # raise RecordNotFound exception (404 error) if not found @post = current_user.posts.find(params[:id]) end end
こうすることで、current_user.postsにだけ紐づくpost、言い換えるとcurrent_userに紐づくことが保証されたpostを検索することができた。 紐付いたpostが存在しない場合、404エラーが返却される。 current_userとオーナーを比較する必要は全くなく、スコープアクセスを使うだけでアクセス権のチェックをより単純に書くことができる。
感想
404が返るってところは、厳密に言うとActiveRecord::RecordNotFoundがraiseされて、rescue_fromを書いていれば、ActionController::Rescueがひろう仕組みらしい。