Topic: refactoring out some duplication

Hi rails gurus!

Got a refactoring problem here... can it be done?

For all of you who experienced Ryan Bates's amazing Railscast #103, you can understand my desire to jump right in an use it on my site.  I added it yesterday and also improved it.  I pulled the announcements div out into a partial, renamed one of the methods, and added a method to check if more than one message was being displayed, and if so, produce a plural form of the hide statement.

In adding the method, I made messy looking model and I need help refactoring it please.

Here is all the code I have changed/added to Ryan's examples in the railscast episode and notes...

[code type=ruby]
######## in application.html.erb - applicaion layout

<%= render :partial => "shared/announcements" unless announcements_to_display.empty? %>

######## new _announcements.html.erb - the announcements partial

<div id="announcement">
  <% for announcement in announcements_to_display %>
    <p><%=h announcement.message %></p>
  <% end %>
  <p><%= announcements_hide_link %></p>
</div>

######## in application_helper.rb

  #changed the name of this method
  def announcements_to_display
    @announcements_to_display ||= Announcement.current_announcements(session[:announcement_hide_time])
  end
 
  #here is my new helper method to product the link
  def announcements_hide_link
    if Announcement.count_all_current_announcements(session[:announcement_hide_time]) > 1
      message = "Hide these messages"
    else
      message = "Hide this message"
    end
    link_to_remote message, :url => "/javascripts/hide_announcement.js"
  end

######## in announcements.rb

  with_scope :find => { :conditions => "starts_at <= current_timestamp AND ends_at >= current_timestamp" } do
    def self.current_announcements(hide_time)
      if hide_time.nil?
        find(:all)
      else
        find(:all, :conditions => ["updated_at > ? OR starts_at > ?", hide_time, hide_time])
      end
    end

    def self.count_all_current_announcements(hide_time)
      if hide_time.nil?
        count
      else
        count(:conditions =>  ["updated_at > ? OR starts_at > ?", hide_time, hide_time])
      end
    end
  end
[/code]

I need help with the major duplication in the model.  How can I count in a better way?  Also, if any of you have other suggestions, I am all ears!

Thanks for the help, and thanks to Ryan for the awesome railscasts.

Last edited by avarhirion (2008-05-09 14:16:21)

Re: refactoring out some duplication

Okay, so how lame am I to be replying to my own post?

As you'll recall from the original post, I was asking for help refactoring the code because of duplication, specifically in the model.

So the reason I came to you guys in the first place is because the method I thought I could use, the "count" method, wasn't available outside of the model, so I wrote that complex and repetitive code in my model to make the thing, and then hated my work because it was dirty.  So  then I asked for help refactoring it.  Tonight I was doing some research for a different aspect of my site and found the "size" method.  Well duh!  I had seen that before, but I forgot.

Suddenly, a lot changed.  The size method does exactly what I wanted the count method to do and I could access it where I needed it!

By being using the size method I now longer needed my dirty code in the model.  With a few minor changes I made in my method and variable names, here is my code refactored.

######## in application_helper.rb

  def announcements_to_display
    @announcements_to_display ||= Announcement.current_announcements(session[:announcement_hide_time])
  end
 
  def announcements_hide_link
    if @announcements_to_display > 1
      text = "Hide these messages"
    else
      text = "Hide this message"
    end
    link_to_remote text, :url => "/javascripts/hide_announcement.js"
  end

######## in announcements.rb

  def self.current_announcements(hide_time)
    with_scope :find => { :conditions => "starts_at <= current_timestamp AND ends_at >= current_timestamp" } do
      if hide_time.nil?
        find(:all)
      else
        find(:all, :conditions => ["updated_at > ? OR starts_at > ?", hide_time, hide_time])
      end
    end
  end


What a difference!

Note that I was now able to move the with_scope back to its original location from Ryan Bates's source. 

So, if you are a newbie like I am, I suggest you heed my warning: if you think that logically it should exist in the framework, then it probably does.  smile

~

Now I have one more thought... and again I ask your opinion.  For readability, it seems like a ruby/rails convention is to write skinny code, yes, but also write human readable code.  Following that ideal, should I extract the if...then logic from the announcements_hide_link method into its own method?  I have done so in the sample below, and I wonder if the readability benefit justifies the additional lines of code (3 lines)?

######## in application_helper.rb

  def announcements_to_display
    @announcements_to_display ||= Announcement.current_announcements(session[:announcement_hide_time])
  end
 
#  def announcements_hide_link
#    if @announcements_to_display > 1
#      text = "Hide these messages"
#    else
#      text = "Hide this message"
#    end
#    link_to_remote text, :url => "/javascripts/hide_announcement.js"
#  end

  def announcements_hide_link
    link_to_remote announcements_hide_link_text(@announcements_to_display.size), :url => "/javascripts/hide_announcement.js"
  end

  def announcements_hide_link_text(size)
    if size > 1
      "Hide these messages"
    else
      "Hide this message"
    end
  end