Please enlighten me and break me out of my shell

rubyonrails-talk
1

One of the hardest things I find myself doing (being a newbie to rails) is getting back into the long frame of mind with other languages I program in.

Take for instance this method:

def self.do_sort(sort_by, search, page)     start_date = Time.now.beginning_of_week     end_date = Time.now.end_of_week     if (sort_by != "all") then       paginate :per_page => 20, :page => page,         :conditions => ['compiled_on > ? and compiled_on < ?', start_date, end_date],         :conditions => ['name like ?', "%#{search}%"],         :order => 'rank'     else       paginate :per_page => 120, :page => page,         :conditions => ['compiled_on > ? and compiled_on < ?', start_date, end_date],         :conditions => ['name like ?', "%#{search}%"],         :order => 'rank'     end   end

It works fine but again, it doesn't appear DRY because the conditions match for both sides of the equation. The only things I'm changing are the page parameters.

How do I make this dry and pretty?

2

Älphä Blüë wrote: [...]

The only things I'm changing are the page parameters.

How do I make this dry and pretty?

As I've advised others on this list: get your head out of the Rails API for a moment and remember that the parameters are plain old hashes. That means you can put the common parameters in a local variable and then use Hash#merge or #= to set ;page. Very simple if you know your basic Ruby functions. :slight_smile:

Best,

3

Here's a simplification -> http://gist.github.com/129084

You're not using this sort_by "all" for anything in this code, why is it in there?

The named scope thing you'll see in the code is explained here - http://ryandaigle.com/articles/2008/3/24/what-s-new-in-edge-rails-has-finder-functionality

4

Okay here's what I did and yes, it's a lot shorter (thanks!):

  named_scope :compiled_this_week, lambda { { :conditions => ['compiled_on > ? and compiled_on < ?', Time.now.beginning_of_week, Time.now.end_of_week] } }

  def self.do_sort(sort_by, search, page)     (sort_by == "all") ? numpages = 120 : numpages = 20     compiled_this_week.paginate( :conditions => ['name like ?', "%#{search}%"], :order => 'rank', :per_page => numpages, :page => page )   end

5

Your ActiveRecord models have nothing to do with the application controller, you can't define a named_scope on it either.

Named scopes can only be defined at ActiveRecord objects.

6

This whole "sort_by" thing isn't really nice, bad naming and an even worse condition, here's how you can avoid this if and even improve the method name that says nothing about what's being searched.

http://gist.github.com/129084

7

Okay, I kept the scope the same..

I actually liked both of your ideas and gives me a lot more functionality with the method. This is what I went with:

  def self.list(search, page, per_page = 20, fetch = nil)     per_page = 120 if (fetch == "all")     compiled_this_week.paginate :conditions => ['name like ?', "%#{search}%"], :order => 'rank', :per_page => per_page, :page => page   end

I kept the "search" in because the name of the team(s) are being returned via a clickable search button. So, it made sense to keep that listed as search. I might change that though.

The issue I have is when I bring up my default page, it lists (30 teams). Now, I'm not sure why it's doing that. I have the per_page set to 20.

Here's an example:

#-- Rushing Offenses Controller

@rushing_offenses = RushingOffense.list(params[:search], params[:page], params[:per_page], params[:fetch])

#-- Rushing Offenses Model

named_scope :compiled_this_week, lambda { { :conditions => ['compiled_on

? and compiled_on < ?', Time.now.beginning_of_week,

Time.now.end_of_week] } }

  def self.list(search, page, per_page = 20, fetch = nil)     per_page = 120 if (fetch == "all")     compiled_this_week.paginate :conditions => ['name like ?', "%#{search}%"], :order => 'rank', :per_page => per_page, :page => page   end

#-- Rushing Offenses index.html

         <%= page_entries_info @rushing_offenses, :entry_name => 'Team' %>         </div>         <%= link_to "Show All Teams", rushing_offenses_path(:fetch => "all") %>         <%= will_paginate @rushing_offenses, :prev_label => '<<', :next_label => '>>', :container => false %>       </div>

8

That's probably 'cos you're setting the per_page parameter to nil in your controller.

And please remove this "fetch" param. it's useless, use only the per_page parameter for that.

9

A lot shorter now:

== Rushing Offenses Model ==

  named_scope :compiled_this_week, lambda { { :conditions => ['compiled_on > ? and compiled_on < ?', Time.now.beginning_of_week, Time.now.end_of_week] } }

  def self.list(search, page, numteams = 20)     compiled_this_week.paginate :conditions => ['name like ?', "%#{search}%"], :order => 'rank', :per_page => numteams, :page => page   end

== Rushing Offenses Controller ==

@rushing_offenses = RushingOffense.list(params[:search], params[:page], params[:numteams])

== Rushing Offenses index.html ==

        <%= link_to "Show All Teams", rushing_offenses_path(:numteams => 120) %>         <%= will_paginate @rushing_offenses, :prev_label => '<<', :next_label => '>>', :container => false %>

10

Right from the will_paginate RDOCs..

:per_page — defaults to CurrentModel.per_page (which is 30 if not overridden)

I thought by placing in the variable for numteams = 20 and applying that to the per_page it would override it...

11

In order to override the default of 30 that will_paginate enforces I had to do the following:

  def self.list(search, page, numteams)     numteams = 20 if (numteams == nil)     compiled_this_week.paginate :conditions => ['name like ?', "%#{search}%"], :order => 'rank', :per_page => numteams, :page => page   end

This works..