5
\$\begingroup\$

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?

\$\endgroup\$
2
\$\begingroup\$

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

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

\$\endgroup\$
  • \$\begingroup\$ 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) \$\endgroup\$ – GhostRider May 6 '15 at 17:39
  • \$\begingroup\$ @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. \$\endgroup\$ – Greg Burghardt May 6 '15 at 20:42
  • \$\begingroup\$ @GregBurghardt This right there is mind blowing for someone who has done Java a lot and now is learning Ruby. \$\endgroup\$ – Marc-Andre May 6 '15 at 23:23
  • \$\begingroup\$ 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 ! \$\endgroup\$ – m_x Jul 7 '15 at 13:54
  • \$\begingroup\$ @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. \$\endgroup\$ – Greg Burghardt Jul 7 '15 at 14:47

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.