patternrubyrailsMinor
Is my blog controller too "Fat"?
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
This action does a number of things.
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?
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)
endThis action does a number of things.
- It shows a micropost
- It gets all the tags (acts as taggable) from the micropost (to display them)
- It builds the response - response form is on the same page
- It impressions the micropost (impressionist gem)
- 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
endThe 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
Then your controller becomes:
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).
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
endThen 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
endIt'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
enddef 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
endContext
StackExchange Code Review Q#89982, answer score: 2
Revisions (0)
No revisions yet.