# Is my blog controller too "Fat"?

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?

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 < 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


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).

• One question - this works beautifully, but what I don't get is, in your method, you use tag_list without calling it on an object . Because your method is going to be called on the object @micropost, does tag_list as you have used it, refer to that object? This is really just a basic syntax question (if you have time) May 6, 2015 at 17:39
• @GhostRider: You are correct. Since self refers to a Micropost object, and this class has a method called tag_list, and no local variable in that method has the same name, Ruby implicitly calls self.tag_list without you needing to specify the object. May 6, 2015 at 20:42
• @GregBurghardt This right there is mind blowing for someone who has done Java a lot and now is learning Ruby. May 6, 2015 at 23:23
• about the IDE warning : this is actually an advice from Sandi Metz. The rules are indeed hard to follow, but I can assure you that striving to comply will make you learn a lot of things. Give it a try !
– m_x
Jul 7, 2015 at 13:54
• @m_x: This particular warning occurs because the controller method is calling another method on the same controller object. It's trying to guard against one controller action calling another action, which the OP is not doing --- hence the "grain of salt" comment about IDE suggestions. Jul 7, 2015 at 14:47