9
\$\begingroup\$

This code for my blog checks to see if tags are in the params hash. If they are, then only posts that are tagged will be paginated. Otherwise, all of the posts are paginated.

class PostsController < ApplicationController
  def index
    if params[:tag]
      @posts = Post.tagged_with(params[:tag]).paginate(page: params[:page])
    else
      @posts = Post.all.paginate(page: params[:page])
    end
  end
end

I feel like this checking of params shouldn't be the concern of the controller, but of some other model like PostParameterChecker. How do you feel about this? Where does this code actually belong?

\$\endgroup\$
9
\$\begingroup\$

Creating a model just to check the parameters is overkill and not idiomatic in Rails. If you need some extra logic, use the existing model. In this case, I'd make Post#tagged_with accept a nil value which wouldn't filter anything:

def Post
  scope :tagged_with, ->(tag) { tag ? <code to filter> : all }
end

class PostsController < ApplicationController
  def index
    @posts = Post.tagged_with(params[:tag]).paginate(page: params[:page])
  end
end
\$\endgroup\$
  • \$\begingroup\$ What is that ... supposed to mean? \$\endgroup\$ – 200_success Sep 13 '14 at 22:51
2
\$\begingroup\$

I'd say that this does belong in the controller. It's the controller's job to handle requests (including params) and prepare the view - which is what your code does.

That said, I wouldn't be opposed to tokland's suggestion of moving the logic to the scope. But personally I would rather choose whether or not to call a scope method at all, than let the scope handle nils by basically doing nothing. It's largely a matter of opinion, though. (You could even move the logic in the other direction, so to speak, and add a /posts/tagged route and wholly separate action.)

Anyway, you can tweak the action a bit:

def index
  @posts = params[:tag] ? Post.tagged_with(params[:tag]) : Post.all
  @posts = @posts.paginate(page: params[:page])
end

or, avoiding the variable re-assignment (which is also avoidable by just using two different variables, of course):

def index
  @posts = if params[:tag]
    Post.tagged_with(params[:tag])
  else
    Post.all
  end.paginate(page: params[:page]
end

It could still be a ternary of course.

In general though, "skinny controller, fat model" is a good rule of thumb. But for things like this I think the controller still has a role to play.

\$\endgroup\$
  • \$\begingroup\$ Thanks for the thorough answer, Flambino! I haven't tested the code yet, but wouldn't line 3 of your first example have to be @posts = posts.paginate(page: params[:page])? \$\endgroup\$ – Dylan Richards Sep 13 '14 at 23:06
  • \$\begingroup\$ @DylanRichards Yes, of course. I've fixed my answer. While it's sort of frowned upon, I'd be ok with re-assigning @posts in this case (as in my now-fixed answer) \$\endgroup\$ – Flambino Sep 13 '14 at 23:14

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.