current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 5 down vote favorite

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?

share|improve this question

2 Answers 2

up vote 2 down vote accepted

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.

share|improve this answer
    
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])? –  Dylan Richards Sep 13 '14 at 23:06
    
@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) –  Flambino Sep 13 '14 at 23:14
up vote 3 down vote

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
share|improve this answer
    
What is that ... supposed to mean? –  200_success♦ Sep 13 '14 at 22:51
    
... is the actual code of how the filtering by tag is performed (which it's not in the question, hence the placeholder). –  tokland Sep 13 '14 at 22:53

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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