無駄に深いネスト
元記事はこちら
翻訳
Bad Smell
map.resources :posts do |post| post.resources :comments do |comment| comment.resources :favorites end end <%= link_to post_comment_favorite_path(@post, @comment, @favorite) %>
3階層にネストしたルーティングは本当に必要でしょうか?ルーティングをネストするということは、子階層のリソースが親階層のリソースに依存しているということを意味します。 上記の例では、commentsというリソースがpostに依存しているということです。 しかし、favoritesはcommentに依存するものですから、favoritesのリソースがpostに依存している必要は全くありません。
Refactor
map.resources :post do |post| post.resources :comments end map.resources :comments do |comment| comment.resources :favorites end <%= link_to comment_favorite_path(@comment, @favorite) %>
以上の理由から、3階層のネストを2階層に減らしたルーティングにリファクタリングしました。favoritesへのurlもこうすることで短くなります。
Updated by @flyerhzm
無駄に深いネストを回避する新しい方法です。
Refactor 2
map.resources :posts, :shallow => true do |post| post.resources :comments do |comment| comment.resources :favorites end end <%= link_to comment_favorite_path(@comment, @favorite) %>
見ての通り、shallow => true
オプションをpostのルーティングに追加することで、1階層か2階層以内に抑えてルーティングを生成します。
以下のように生成されます。
posts GET /posts(.:format) {:action=>"index", :controller=>"posts"} POST /posts(.:format) {:action=>"create", :controller=>"posts"} new_post GET /posts/new(.:format) {:action=>"new", :controller=>"posts"} edit_post GET /posts/:id/edit(.:format) {:action=>"edit", :controller=>"posts"} post GET /posts/:id(.:format) {:action=>"show", :controller=>"posts"} PUT /posts/:id(.:format) {:action=>"update", :controller=>"posts"} DELETE /posts/:id(.:format) {:action=>"destroy", :controller=>"posts"} post_comments GET /posts/:post_id/comments(.:format) {:action=>"index", :controller=>"comments"} POST /posts/:post_id/comments(.:format) {:action=>"create", :controller=>"comments"} new_post_comment GET /posts/:post_id/comments/new(.:format) {:action=>"new", :controller=>"comments"} edit_comment GET /comments/:id/edit(.:format) {:action=>"edit", :controller=>"comments"} comment GET /comments/:id(.:format) {:action=>"show", :controller=>"comments"} PUT /comments/:id(.:format) {:action=>"update", :controller=>"comments"} DELETE /comments/:id(.:format) {:action=>"destroy", :controller=>"comments"} comment_favorites GET /comments/:comment_id/favorites(.:format) {:action=>"index", :controller=>"favorites"} POST /comments/:comment_id/favorites(.:format) {:action=>"create", :controller=>"favorites"} new_comment_favorite GET /comments/:comment_id/favorites/new(.:format) {:action=>"new", :controller=>"favorites"} edit_favorite GET /favorites/:id/edit(.:format) {:action=>"edit", :controller=>"favorites"} favorite GET /favorites/:id(.:format) {:action=>"show", :controller=>"favorites"} PUT /favorites/:id(.:format) {:action=>"update", :controller=>"favorites"} DELETE /favorites/:id(.:format) {:action=>"destroy", :controller=>"favorites"}
:shallowは良い解決策だと思います。このオプションにより、以下のような使い方ができます。
post_comment_path(1, 1) comment_path(1) comment_favorite_path(1, 1) favorite_path(1)
素晴らしい!でもrailsドキュメントに載っていないのが残念です。
感想
railsドキュメントに載っていない 掲載されるようになったみたい。 ActionDispatch::Routing::Mapper::Resources
これも読んで、できる限りRESTfulなルーティングに近づけるようにしていると、1アクションの責務がとても明確になって良い。 postd.cc
model.collection_model_ids (多対多)
元記事はこちら
翻訳
Bad Smell
class User < ActiveRecord::Base has_many :user_role_relationships has_many :roles, :through => :user_role_relationships end class UserRoleRelationship < ActiveRecord::Base belongs_to :user belongs_to :role end class Role < ActiveRecord::Base end <% form_for @user do |f| %> <%= f.text_field :email %> <% Role.all.each |role| %> <%= check_box_tag 'role_id[]', role.id, @user.roles.include?(role) %> <%= role.name %> <% end %> <% end %> class UsersController < ApplicationController def update @user = User.find(params[:id]) if @user.update_attributes(params[:user]) @user.roles.delete_all (params[:role_id] || []).each { |i| @user.roles << Role.find(i) } end end end
見ての通り、UserモデルとRoleモデルが多対多の関係で、1ユーザーに複数のroleを設定できるフォームがあるとします。 コントローラーではまず、ユーザーの権限を全て削除してから、ユーザーがチェックボックスで選択した権限を割り当てています。 自前で権限の割り当てをするのは見苦しいので、railsの仕組みにのるべきです。
Refactor
<% form_for @user do |f| %> <% Role.all.each |role| %> <%= check_box_tag 'user[role_ids][]', role.id, @user.roles.include?(role) %> <%= role.name %> <% end %> <%= hidden_field_tag 'user[role_ids][]', '' %> <% end %> class UsersController < ApplicationController def update @user = User.find(params[:id]) @user.update_attributes(params[:user]) # the same as @user.role_ids = params[:user][:role_ids] end end
チェックボックスの名前を"role_id“ から "user[role_ids]"に変更しました。こうすることで、role_idsは自動的にUserモデルに割り当てられます。 hidden_field_tag に'user[role_ids][]‘があることに注意してください。値は空で、ユーザー権限のチェックボックスが選択されなかった場合、権限が全て削除されることを保証します。 これでコントローラーが簡潔になりました。
感想
Rails4~だとStrongParams使うので、コントローラーは以下のようになるはず。
class HomeController < ApplicationController def update p params @user = User.find(params[:id]) @user.update_attributes(role_params) # the same as @user.role_ids = params[:user][:role_ids] end private def role_params params.require(:user).permit(:role_ids) end end
モデルのコールバックを使う
元記事はこちら
翻訳
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の誤り?