Rails - Jun 2009 - 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?
-- 
Posted via http://www.ruby-forum.com/.

Ä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. :)

Best,
--
Marnen Laibow-Koser
http://www.marnen.org
marnen-sbuyVjPbboAdnm+yROfE0A@public.gmane.org
-- 
Posted via http://www.ruby-forum.com/.

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
-- 
Posted via http://www.ruby-forum.com/.

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



On Sat, Jun 13, 2009 at 12:21 AM, Älphä
Blüë<rails-mailing-list-ARtvInVfO7ksV2N9l4h3zg@public.gmane.org>
wrote:
>
> 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
> --
> Posted via http://www.ruby-forum.com/.
>
> >
>

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.

-- 
Posted via http://www.ruby-forum.com/.

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

-- 
Posted via http://www.ruby-forum.com/.

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?

-- 
Posted via http://www.ruby-forum.com/.

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



On Sat, Jun 13, 2009 at 10:46 AM, Älphä
Blüë<rails-mailing-list-ARtvInVfO7ksV2N9l4h3zg@public.gmane.org>
wrote:
>
> 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?
>
> --
> Posted via http://www.ruby-forum.com/.
>
> >
>

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



On Sat, Jun 13, 2009 at 10:19 AM, Älphä
Blüë<rails-mailing-list-ARtvInVfO7ksV2N9l4h3zg@public.gmane.org>
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] } }
>
>  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
>
> --
> Posted via http://www.ruby-forum.com/.
>
> >
>

Ä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
marnen-sbuyVjPbboAdnm+yROfE0A@public.gmane.org
-- 
Posted via http://www.ruby-forum.com/.

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.

-- 
Posted via http://www.ruby-forum.com/.

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>

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


-- 
Posted via http://www.ruby-forum.com/.

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



On Sat, Jun 13, 2009 at 11:47 AM, Älphä
Blüë<rails-mailing-list-ARtvInVfO7ksV2N9l4h3zg@public.gmane.org>
wrote:
>
> 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>
>
> ==========================================>
> 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.
>
>
> --
> Posted via http://www.ruby-forum.com/.
>
> >
>

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

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

-- 
Posted via http://www.ruby-forum.com/.

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

-- 
Posted via http://www.ruby-forum.com/.

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..
-- 
Posted via http://www.ruby-forum.com/.

Ä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
marnen-sbuyVjPbboAdnm+yROfE0A@public.gmane.org
-- 
Posted via http://www.ruby-forum.com/.

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! :)

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

The beginning template is working out nicely.

-- 
Posted via http://www.ruby-forum.com/.