Please enlighten me and break me out of my shell

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?

Ä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,

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

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

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.

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

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>

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.

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 %>

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...

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..