HiveBrain v1.2.0
Get Started
← Back to all entries
patternrubyrailsMinor

Is my blog controller too "Fat"?

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
blogfattoocontroller

Problem

I would like some advice which relates to the "fat model skinny controller" concept in Rails.

See my show action in the Micropost controller

def show
     @micropost = Micropost.find(params[:id])
     @tags = @micropost.tag_counts_on(:tags)
     @responses =  @micropost.responses
     @response = current_user.responses.build if member_signed_in?
     impressionist(@micropost)
     related_posts = Micropost.tagged_with(@micropost.tag_list, :on => :tags, :any => true).take(5)
     @cleaned_related_posts = Micropost.assemble_related_posts(related_posts, @micropost)
 end


This action does a number of things.
  1. It shows a micropost
  2. It gets all the tags (acts as taggable) from the micropost (to display them)
  3. It builds the response - response form is on the same page
  4. It impressions the micropost (impressionist gem)
  5. Lastly, it builds a list of related microposts. I tried to clean this up by using a class method in my micropost class as follows



def self.assemble_related_posts(related_posts = [], micropost)
  @cleaned_related_posts = []
  related_posts.each do |post|
    unless post == micropost
      @cleaned_related_posts.push(post)
    end
   end
  @cleaned_related_posts
end


The related posts variable above INCLUDES the micropost being shown - no point in that, so I wrote a class method to remove it.

This show action is pretty fat, and my RubyMine keeps showing me a warning that the controller should call only one method.

I want to improve my skills (I am fairly new to Rails). Is there a "Rails way" to shift all this logic into the model.....should it be done? Are some controllers simply going to a bit on the fat side?

Solution

The only thing I can see is moving the related/cleaned posts method into an instance method on Micropost. I'm also not sure what the implementation of Micropost.tagged_with is like, but I bet that could be moved into an instance method. Without that, I might suggest:

class Micropost  :tags, :any => true).reject { |p| p == self }
    posts = posts.take(limit) if limit > 0
    posts
  end
end


Then your controller becomes:

def show
  @micropost = Micropost.find(params[:id])
  @tags = @micropost.tag_counts_on(:tags)
  @responses =  @micropost.responses
  @response = current_user.responses.build if member_signed_in?
  impressionist(@micropost)
  @cleaned_related_posts = @micropost.clean_related_posts 5
end


It's not really much of a savings, but it does at least centralize the logic for related posts. This controller isn't really fat to begin with.


... RubyMine keeps showing me a warning that the controller should call only one method.

That's just plain silly. IDE suggestions should be taken with a grain of salt (and a wedge of lime, and a shot of tequila).

Code Snippets

class Micropost < ActiveRecord::Base
  def cleaned_related_posts(limit = -1)
    posts = Micropost.tagged_with(tag_list, :on => :tags, :any => true).reject { |p| p == self }
    posts = posts.take(limit) if limit > 0
    posts
  end
end
def show
  @micropost = Micropost.find(params[:id])
  @tags = @micropost.tag_counts_on(:tags)
  @responses =  @micropost.responses
  @response = current_user.responses.build if member_signed_in?
  impressionist(@micropost)
  @cleaned_related_posts = @micropost.clean_related_posts 5
end

Context

StackExchange Code Review Q#89982, answer score: 2

Revisions (0)

No revisions yet.