Advice on best practices for coding a dynamic controller action

Ruby on Rails
1

Hi!

I’ve written an Index action that will instantiate objects differently based on what selection the user choses. Here’s the code which better illustrates what I’m talking about:

  def index
    review_type = params[:review_type]
    if review_type == "1"
      @count = Review.posted.count
      @reviews = Review.posted
    elsif review_type == "2"
      @count = Review.rejected.count
      @reviews = Review.rejected
    # default to display pending reviews
    else
      @count = Review.pending.count
      @reviews = Review.pending.page
    end
  end

So basically, the index view page provides the user different selections which display different scopes (in this case, pending, rejected, posted) of the same Model. The choice (here, review_type) is passed through with the post method when the user clicks the link.

Is there a better way to handle this case? How can this code be improved? Thanks in advance!

2

Hi @realDLee , I think this qualifies as a “Case Statement” smell, and the solution would be to apply Replace Conditional With Polymorphism. You can check out Ruby Science for examples and advice on refactoring for this. Hope this helps !

3

What about about filtering your reviews by review_type using one scoped method rather than using a case statement in your controller?

4

Thanks for the responses so far! I took a look at the Ruby Science chapter (starts on p39 in the pdf) and am considering that route as well as looking more into what @joannecheng mentioned. Would love to hear more suggestions since this seems like a very common scenario!

5

Hi @realDLee, before you go down the polymorphism route think about your data. Are there really multiple types or have your created convenient scopes that look like types, maybe because of your variable “review_type” The variable name “review_state” might have been better. My guess it that you actually have one type that is changing states, from “pending” to “posted” or “rejected”. If that is the case polymorphism is overkill. Just create one search that uses review_type as a parameter as @joannecheng suggested. If your branching is on Class Types bring on the polymorphism as (pedromoreira) suggested. Good luck!

1 Like
6

@richwiltshire You’re right. They “types” are actually scopes that relate to the review’s state. After re-reading the Ruby Science section, I do think that route might be too much. Thanks!

7

Both @joannecheng and @richwiltshire are right, I should have read your description more carefully. Sorry if it my answer led to any delay in solving your problem :slight_smile: