入れ子モデルフォーム

元記事はこちら

翻訳

Bad Smell

class Product < ActiveRecord::Base
  has_one :detail
end

class Detail < ActiveRecord::Base
  belongs_to :product
end

<% form_for :product do |f| %>
  <%= f.text_field :title %>
  <% fields_for :detail do |detail| %>
    <%= detail.text_field :manufacturer %>
  <% end %>
<% end %>

class ProductsController < ApplicationController
  def create
    @product = Product.new(params[:product])
    @detail = Detail.new(params[:detail])

    Product.transaction do
      @product.save!
      @detail.product = @product
      @detail.save
    end
  end
end

ProductモデルとDetailモデルには1対1の関係があり、productと一緒にmanufacturerも保存したいとします。この場合、productオブジェクトとdetailオブジェクトを生成し、トランザクションによって関連付けます。でも、なんでこんな込み入った処理をコントローラーでやっているのでしょう。Productモデルがdetailの生成を担当すべきです。

Refactor

class Product < ActiveRecord::Base
  has_one :detail
  accepts_nested_attributes_for :detail
end

<% form_for :product do |f| %>
  <%= f.text_field :title %>
  <% f.fields_for :detail do |detail| %>
    <%= detail.text_field :manufacturer %>
  <% end %>
<% end %>

class ProductsController < ApplicationController
  def create
    @product = Product.new(params[:product])
    @product.save
  end
end

Productモデルに、accepts_nested_attributes_forを追加したので、ProductモデルはDetailモデルの生成を制御できるようになりました。これは良いことです!

accepts_nested_attributes_forのおかげで、1対多の関係における生成も簡単にできるようになります。

class Project < ActiveRecord::Base
  has_many :tasks
  accepts_nested_attributes_for :tasks
end

class Task < ActiveRecord::Base
  belongs_to :project
end

<% form_for @project do |f| %>
  <%= f.text_field :name %>
  <% f.fields_for :tasks do |tasks_form| %>
    <%= tasks_form.text_field :name %>
  <% end %>
<% end %>

railsに素晴らしいメソッドが用意されていて、実にありがたいことです!

感想

Railsガイドの関連付けに関する章を最近読みながら書いてますが、いまいち理解できていない。 関連付けの仕方はわかってきたものの、生成のさせ方と検索時に取れるレコードの形(Assosiation/Array)の判別がいまいち腑に落ちていないというか、モヤっとしている。 今日もちょうどBadSmellなコードを書いてしまったので、テスト書いてリファクタリングせねば。。

無駄に深いネスト

元記事はこちら

翻訳

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行が一つの基準になる?

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

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