CODEWITHSUNDEEP
ruby-on-railsrubyruby-on-rails-3
Matt Elhotiby
Matt Elhotiby

Reputation: 44066

Is there a better way to do this in ruby

Actually this currently doesnt work at all

 @category = Category.find(params[:id])
 @sub_categories = @category.sub_categories
 if @sub_categories
  @designs = []
  @sub_categories.each do |sub_cat|
    @designs << sub_cat.designs.paginate :page => params[:page], :order => 'name', :per_page => @per_page
  end
end

Its failing on this syntax error

categories_controller.rb:21: syntax error, unexpected tSYMBEG, expecting kEND
...<< sub_cat.designs.paginate :page => params[:page], :order ...

Basically i have category has many sub_categories which has many designs and its on the current category and I want to show all the designs for that category...any idea on best practices and how too fix this issue

Upvotes: 3

Views: 173

Answers (4)

Peter Brown
Peter Brown

Reputation: 51707

I would do two things to refactor this...

First is to reduce coupling by moving the pagination options to a class method on the design model. Your controller shouldn't know this much about the design model.

class Design < ActiveRecord::Base
  def self.paginate_with_params(params, per_page)
    paginate(:page => params[:page], :order => 'name', :per_page => per_page)
  end
end

Second is to remove the unnecessary variables and logic from your controller that don't really add much value:

@category = Category.find(params[:id])
@designs = @category.sub_categories.map |sub_cat|
  sub_cat.designs.paginate_with_params(params, @per_page)
end

I haven't tested this code, so please go easy on me if it doesn't work :)

Upvotes: 2

John Douthat
John Douthat

Reputation: 41179

@category = Category.find(params[:id])
@sub_categories = @category.sub_categories # assumption: this doesn't return nil
page_opts = {:page => params[:page], :order => 'name', :per_page => @per_page}
@designs = @sub_categories.map {|sub_cat| sub_cat.designs.paginate(page_opts) }

Upvotes: 1

Nakilon
Nakilon

Reputation: 35084

a = []
b.each do |c|
    a << c.d(e)
end

Is equal to:

a = b.map { |c| c.d(e) }

And if d doesn't need parameters:

a = b.map &:d

But in your case, I guess, you just need to add () around your parameters hash, as already answered Ryan Bigg.

Upvotes: 1

Ryan Bigg
Ryan Bigg

Reputation: 107728

Assuming you meant to use @designs rather than @patterns:

@category = Category.find(params[:id])
@sub_categories = @category.sub_categories
pagination_options = { :page => params[:page],
                       :order => 'name',
                       :per_page => @per_page
                     }
unless @sub_categories.empty?
  @designs = []
  @sub_categories.each do |sub_cat|
    @designs << sub_cat.designs.paginate(pagination_options)
  end
end

I think what was missing were the brackets for the arguments to the paginate call.

Upvotes: 2

Related Questions