Please enlighten me and break me out of my shell

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

Marnen Laibow-Koser
http://www.marnen.org
[email protected]

3

Marnen, good advice.

Something like this:

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

4

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

Maurício Linhares
http://codeshooter.wordpress.com/ | http://twitter.com/mauriciojr

5

Maurício Linhares wrote:

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

Maur�cio Linhares
http://codeshooter.wordpress.com/ | http://twitter.com/mauriciojr

Thanks mate. That is a good explanation link.

My table has 120 records. What happens is when someone views the page,
it’s going to list 20 records with pagination for the other pages. They
can click a link for “show all” and it lists the full 120 records.
That’s what the code above does. However, it doesn’t sort by anything -
perhaps I should have named it show_all.

I was getting ready to put the sort information in there but I haven’t
gotten that far yet.

I really just want the following things:

A simplified page view showing 20 teams per page with the option to show
all or go back to the simplified page view. In addition, I want all
columns to be sortable (whether it’s in the simplified view or the full
page view.

I like the code example you gave though. I’ll look through it and mull
it over. Please remember (for those helping me) some of what I might do
might not make any sense at all (from a veteran perspective) but I’m
still very newbish when it comes to all of this. I learn better as I
dig deep into code and learn the nuances of it all.

I’m still reading but reading only gets you partially there.

6

I also have a quick question on named_scopes. If all my models are
going to use the compiled_this_week named scope, would it be simpler to
put it in the application controller?

7

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

8

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

Maurício Linhares
http://codeshooter.wordpress.com/ | http://twitter.com/mauriciojr

9

Älphä Blüë wrote:

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] } }

Why not use BETWEEN in your SQL fragment?

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

Here’s my suggestion. Note that this takes care of the improper naming
of the method and variables (do_sort? why?), the illogical order of
arguments (put the optional ones last!), and the use of strings where
symbols (or perhaps booleans) would be more appropriate.

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

Best,

Marnen Laibow-Koser
http://www.marnen.org
[email protected]

10

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.

Maurício Linhares
http://codeshooter.wordpress.com/ | http://twitter.com/mauriciojr

11

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'

%>

<%= link_to “Show All Teams”, rushing_offenses_path(:fetch =>
“all”) %>
<%= will_paginate @rushing_offenses, :prev_label => ‘<<’,
:next_label => ‘>>’, :container => false %>

===========================================

I’m not sure why it’s pulling 30. I’ve been scratching my head at that
one. The Show All Teams shows 120 teams like it’s supposed to. Each
page within the pagination shows a list of 30 teams when it’s supposed
to show 20.

12

thanks guys - I’m learning some things today. Mulling over what both of
you said and will get back to you when I get it worked out.

13

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.

Maurício Linhares
http://codeshooter.wordpress.com/ | http://twitter.com/mauriciojr

14

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…

15

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…

16

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 =>
  1. %>
    <%= will_paginate @rushing_offenses, :prev_label => ‘<<’,
    :next_label => ‘>>’, :container => false %>

=======================

changed per_page variable to numteams (makes more sense)

The code is a lot shorter now and it makes more sense to me. However,
the default page views still show 30 teams instead of 20. Why is it
adding 10 somewhere? I’ve checked all 3 files (what’s listed here is
what’s listed there…)

17

I like that better Marnen.

Wow, thanks to both of you (Marnen and Mauricio) for your help today. I
have learned a tremendous amount of information and it’s actually making
sense to me! :slight_smile:

I even added sets of conditionals to my index so that buttons show when
certain events occur…

The beginning template is working out nicely.

18

Älphä Blüë wrote:

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…

That works, but it would be better to actually override the method in
your model:

def self.per_page
20
end

Best,

Marnen Laibow-Koser
http://www.marnen.org
[email protected]