モデルのロジックをモデルに移動する

元記事はこちら

翻訳

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の誤り？