How to refactor this Ruby on Rails code?

Question

I want to fetch posts based on their status, so I have this code inside my PostsController index action. It seems to be cluttering the index action, though, and I'm not sure it belongs here.

How could I make it more concise and where would I move it in my application so it doesn't clutter up my index action (if that is the correct thing to do)?

if params[:status].empty?
  status = 'active'
else
  status = ['active', 'deleted', 'commented'].include?(params[:status]) ? params[:status] : 'active'
end

case status
when 'active'
  #active posts are not marked as deleted and have no comments
  is_deleted = false
  comments_count_sign = "="
when 'deleted'
  #deleted posts are marked as deleted and have no comments
  is_deleted = true
  comments_count_sign = "="
when 'commented'
  #commented posts are not marked as deleted and do have comments
  is_deleted = false
  comments_count_sign = ">"
end

@posts = Post.find(:all, :conditions => ["is_deleted = ? and comments_count_sign #{comments_count_sign} 0", is_deleted])

zed_0xff · Accepted Answer · 2010-06-02 22:06:55Z

2

class Post < ActiveRecord::Base
  named_scope :active, :conditions => { :is_deleted => false, :emails_count => 0 }
  named_scope :sent, :conditions => ["is_deleted = ? AND emails_count > 0", true]
  ...
end

use it like Post.active.all, Post.active.first, Post.active.each, etc

and then

status = %w'active deleted sent'.include?(params[:status]) : params[:status] : 'active'
@posts = Post.send(status).all

answered Jun 2, 2010 at 22:06

zed_0xff

33.5k8 gold badges55 silver badges72 bronze badges

Sign up to request clarification or add additional context in comments.

Comments

GSP · Accepted Answer · 2010-06-02 22:02:50Z

2

I would consider adding a class method to Post

def file_all_based_on_status status

  # custom logic for queries based on the given status here
  # handling nils and other cases

end

That way your Controller index is simple

def index
  @posts = Post.find_all_based_on_status params[:status]
end

answered Jun 2, 2010 at 22:02

GSP

3,7893 gold badges35 silver badges60 bronze badges

2 Comments

Yuval Karmi Over a year ago

thanks! I think you meant def find_all_based_on_status status not def file_all_based_on_status status. Muscle memory at work. Also, is there a way to make the find code itself more concise?

Daniel Vandersluis Over a year ago

Make your finder method use named scopes like @zed_0xff's answer

Collectives™ on Stack Overflow

How to refactor this Ruby on Rails code?

2 Answers 2

Comments

2 Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

Comments

2 Comments

Your Answer

Sign up or log in

Post as a guest

Related