rails can this model code be refactored?

Question

I would like to combine the finders better.

if params[:sort]
  if params[:sort] == 'industry_id'
    if params[:industry_id]
      @companies = Company.find_all_by_industry_id(params[:industry_id], :joins => "left join industries ind on industry_id = ind.id", :order => "ind.name" + " " + sort_direction)
    else
      @companies = Company.all(:joins => "left join industries ind on industry_id = ind.id", :order => "ind.name" +  " " + sort_direction)
    end
  else
    if params[:industry_id]
      @companies = Company.find_all_by_industry_id(params[:industry_id], :order => sort_column + " " + sort_direction)
    else
      @companies = Company.all(:order => sort_column + " " + sort_direction)
    end
  end
else
  if params[:industry_id]
    @companies = Company.find_all_by_industry_id(:joins => "left join industries ind on industry_id = ind.id", :order => "ind.name" + " " + sort_direction)
  else
    @companies = Company.all
  end
end

Is there supposed to be an industry_id somewhere in this line of code: Company.find_all_by_industry_id(:joins => "left join industries ind on industry_id = ind.id", :order => "ind.name" + " " + sort_direction) ? — Adam Eberlin
– Adam Eberlin, Commented Sep 21, 2011 at 3:38

Harish Shetty · Accepted Answer · 2011-09-21 05:19:24Z

4

Assuming you have following model:

class Company
  belongs_to :industry

  def self.search p
    opt = {:conditions => {}}
    opt[:order] = (p[:sort] == "industry_id") ? "industries.name" : p[:sort]
    opt[:order]+= " " + (p[:sort_direction] || "ASC") if opt[:order].present?
    opt[:conditions][:industry_id] = p[:industry_id] if p[:industry_id].present? 
    if (p[:sort] == 'industry_id') or p[:industry_id].present?
      opt[:include] = :industry
    end
    all(opt)
  end
end

class Industry
  has_many :companies
end

Now you can use the search method in your controller:

Company.search(params)

answered Sep 21, 2011 at 5:19

Harish Shetty

64.4k21 gold badges154 silver badges201 bronze badges

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

2 Comments

Michael Durrant Over a year ago

I just switched to this. Love a model based approach. Much better for my rspec testing too.

Michael Durrant Over a year ago

Note - I changed p[:sort_direction] to p[:sort_direction] as my view passes sort_direction in as a parameter. Make sure you match those appropriately if using this answer :) Michael.

mu is too short · Accepted Answer · 2011-09-21 04:35:10Z

2

Am I missing something or does it all boil down to this?

sort_joins = { 'ind.name' => 'left join industries ind on industry_id = ind.id' }
if params[:sort] == 'industry_id' || !params[:sort]
  sort_column = 'ind.name'
end

sort_column = nil if sort_column && !params[:industry_id]

options = { }
if sort_column
  options[:sort]  = "#{sort_column} #{sort_direction}"
  options[:joins] = sort_joins[sort_column] if sort_joins[sort_column]
end
if params[:industry_id]
  @companies = Company.find_all_by_industry_id(params[:industry_id], options)
else
  @companies = Company.all(options)
end

answered Sep 21, 2011 at 4:35

mu is too short

436k71 gold badges863 silver badges822 bronze badges

4 Comments

Michael Durrant Over a year ago

Thanks mu. I always appreciate your answers and I was very grateful when I saw your replies on my q's :)

mu is too short Over a year ago

@Michael: Pushing it down to the model (as suggested by KandadaBoggu) is also a good idea.

Michael Durrant Over a year ago

Yes, model based is certainly better. I did find yours a bit more readable though so it was a close call! I wonder if KandadaBoggu's could be reduced a bit, maybe some ternary's, etc. if still readable.

Michael Durrant Over a year ago

Actually I had a few problems and could not get this to work. In the end I tried the model based approach from KandadaBoggu's and it worked better. Some, hmmm, I am going to switch my answer to that. I hope you don't mind as you did suggest it too. Sorry I don't have time to dig into what the issues where but I kept getting unknown key sort or columns just didn't sort and I just don't have time to dig further. But as you say model based is better :)

Collectives™ on Stack Overflow

rails can this model code be refactored?

2 Answers 2

2 Comments

4 Comments

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

2 Comments

4 Comments

Related