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モデルが多対多の関係で、１ユーザーに複数の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の誤り？

モデルに仮想的な属性を追加する

元記事はこちら

翻訳

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=という仮想的な属性が付加され、姓と名を設定するするようになりました。 こうすれば、たった１行でuserを作成することができるようになり、Userモデルの中で姓と名を分割できるようになるので、いい感じになります。

感想

分割して入力された電話番号をつなげてDBに放り込みたいとか、例とは逆の方向でなら使う機会は多そう。 Railsガイドによると、confirmation: true な時にも仮想属性が生まれてるみたい。

Active Record バリデーション | Rails ガイド