Topic: Writing cleaner Ruby code

Hi all,

I'm struggling a bit with keeping the code simple and short in my controller. Basically I've got a submit_policy_numbers action in my proposals_controller.rb for updating a several proposals at the same time with a policy number and/or commission on date. The commission on date is selected like mm/yyyy but that's not so important.

The code below works but it looks damn ugly. How can I refactor it and at the same time maybe use a before filter to avoid the first if condition?

  def submit_policy_numbers
    numbers_h = params[:policy_numbers]
    dates_h = params[:commission_dates]
    if numbers_h.nil? && dates_h.nil?
      redirect_to :action => 'find'
    else
      id_array = Array.new
      id_array.concat(numbers_h.keys) unless numbers_h.nil?
      id_array.concat(dates_h.keys) unless dates_h.nil?
      update_proposals = Proposal.find(id_array)
      update_proposals.each { |p|
        attributes_h = Hash.new
        attributes_h[:policy_number] = numbers_h["#{p.id}"] if !numbers_h.nil? && numbers_h.key?("#{p.id}")
        attributes_h[:commission_on] = Date.new(dates_h["#{p.id}"][:year].to_i, dates_h["#{p.id}"][:mon].to_i, dates_h["#{p.id}"][:day].to_i) if !dates_h.nil? && dates_h.key?("#{p.id}")
        logger.debug "The attributes " + attributes_h.to_s
        p.update_attributes(attributes_h)
      }
      dates_array = Array.new
      dates_h.each_value { |d| dates_array.push("#{d[:mon]}/#{d[:year]}") } unless dates_h.nil?
      flash_notice = "Successfully updated proposals with "
      flash_notice += "policy numbers #{numbers_h.values.join(', ')} " unless numbers_h.nil?
      flash_notice += "and " if !numbers_h.nil? && !dates_h.nil?
      flash_notice += "commission on dates #{dates_array.join(', ')}" unless dates_h.nil?
      flash_notice += "."
      flash[:notice] = flash_notice
      redirect_to :action => 'list'
    end
  end

The flash notice just gives a string that includes all the updated policy numbers and the commission on dates (or one of the two).

The below helper methods used in the form look ok to me and give some information about the POST parameters.

module ProposalsHelper
  def list_contracts(contracts)
  end
 
  def show_policy_number(policy_number, id)
    if policy_number.blank?
      text_field_tag "policy_numbers[#{id}]", '', :size => 6
    else
      "#{policy_number}"
    end
  end
 
  def show_commission_on(commission_on, id)
    if commission_on.nil?
      html = hidden_field_tag("commission_dates[#{id}][day]", "#{Date.today.day}")
      html += select_month(Date.today, :prefix => "commission_dates[#{id}]", :field_name => "mon", :use_month_numbers => true)
      html += select_year(Date.today, :prefix => "commission_dates[#{id}]", :field_name => "year", :start_year => 1989, :end_year => Date.today.year)
    else
      "#{commission_on.mon}/#{commission_on.year}"
    end
  end
end

Thanks for any suggestion!

Mark

Re: Writing cleaner Ruby code

Good call on the before filter. You can do it like this:

# in controller
before_filter :check_policy_numbers

#...

private

def check_policy_numbers
  if params[:policy_numbers].nil? && params[:commission_dates].nil?
    redirect_to :action => 'find'
    false # return false so the action isn't called after this
  end
end


The number one way to simplify code in a controller is to move it into a model (think of skinny controller, fat model). Since we are working with many proposals here, we can make a class method in Proposal to handle the updating. Start with the interface for this new method, something like:

Proposal.update_policy_numbers(params[:policy_numbers], params[:commission_dates])

Now move as much code as you can into this method (as long as it's applicable). Here's the result.

# in controller
def submit_policy_numbers
  Proposal.update_policy_numbers(params[:policy_numbers], params[:commission_dates])
 
  numbers_h = params[:policy_numbers]
  dates_h = params[:commission_dates]
  dates_array = Array.new
  dates_h.each_value { |d| dates_array.push("#{d[:mon]}/#{d[:year]}") } unless dates_h.nil?
  flash_notice = "Successfully updated proposals with "
  flash_notice += "policy numbers #{numbers_h.values.join(', ')} " unless numbers_h.nil?
  flash_notice += "and " if !numbers_h.nil? && !dates_h.nil?
  flash_notice += "commission on dates #{dates_array.join(', ')}" unless dates_h.nil?
  flash_notice += "."
  flash[:notice] = flash_notice
  redirect_to :action => 'list'
end

# in Proposal
def self.update_policy_numbers(numbers_h, dates_h)
  id_array = Array.new
  id_array.concat(numbers_h.keys) unless numbers_h.nil?
  id_array.concat(dates_h.keys) unless dates_h.nil?
  update_proposals = Proposal.find(id_array)
  update_proposals.each { |p|
    attributes_h = Hash.new
    attributes_h[:policy_number] = numbers_h["#{p.id}"] if !numbers_h.nil? && numbers_h.key?("#{p.id}")
    attributes_h[:commission_on] = Date.new(dates_h["#{p.id}"][:year].to_i, dates_h["#{p.id}"][:mon].to_i, dates_h["#{p.id}"][:day].to_i) if !dates_h.nil? && dates_h.key?("#{p.id}")
    logger.debug "The attributes " + attributes_h.to_s
    p.update_attributes(attributes_h)
  }
end


Work still needs to be done in this update_policy_numbers method, but at least  we got the model logic out of the controller, so let's finish cleaning that up then revisit our model.

Another tip to help refactoring is to keep the number of local variables as minimal as possible. As you can see, there's still quite a few in the controller's action. Let's start by getting rid of the dates_array variable by moving it into a method:

def submit_policy_numbers
  Proposal.update_policy_numbers(params[:policy_numbers], params[:commission_dates])
 
  numbers_h = params[:policy_numbers]
  dates_h = params[:commission_dates]
  flash_notice = "Successfully updated proposals with "
  flash_notice += "policy numbers #{numbers_h.values.join(', ')} " unless numbers_h.nil?
  flash_notice += "and " if !numbers_h.nil? && !dates_h.nil?
  flash_notice += "commission on dates #{format_dates_hash(dates_h).join(', ')}" unless dates_h.nil?
  flash_notice += "."
  flash[:notice] = flash_notice
  redirect_to :action => 'list'
end

private

def format_dates_hash(dates)
  dates.collect { |d| "#{d[:mon]}/#{d[:year]}" } unless dates.nil? # this collect method basically does what the push was doing before
end


Getting better, now for the long flash_notice string. I cringe whenever I see concatenating strings like this - probably because I did too much of it in my PHP days. Sometimes you can't avoid it, but here I think it can be done. First to define the interface - this is my goal:

flash[:notice] = "Successfully updated proposals with #{numbers_and_dates_notice(numbers_h, dates_h)}."

Now to define that method:

# in controller
def numbers_and_dates_notice(numbers, dates)
  flash_notice = "policy numbers #{numbers.values.join(', ')} " unless numbers.nil?
  flash_notice += "and " if !numbers.nil? && !dates.nil?
  flash_notice += "commission on dates #{format_dates_hash(dates).join(', ')}" unless dates.nil?
end

Hmm, still using local variables, time to refactor more!

def numbers_and_dates_notice(numbers, dates)
  [policy_numbers_notice(numbers), commission_dates_notice(dates)].compact.join(' and ')
end

def policy_numbers_notice(numbers)
  "policy numbers #{numbers.values.join(', ')}" unless numbers.nil?
end

def commission_dates_notice(dates)
  "commission on dates #{format_dates_hash(dates).join(', ')}" unless dates.nil?
end


This technique of placing the parts of the sentence in an array and calling compact.join() on them is really helpful in removing the local variables. We no longer need an "if" condition for the join.

I'm pretty happy with the controller. The action is only 3 lines now! Yes, the result is a lot of methods, but they all seem fairly descriptive. Of course if you have other date/number methods you'll want to make these a little more specific.

def submit_policy_numbers
  Proposal.update_policy_numbers(params[:policy_numbers], params[:commission_dates])
  flash[:notice] = "Successfully updated proposals with #{numbers_and_dates_notice(params[:policy_numbers], params[:commission_dates])}."
  redirect_to :action => 'list'
end

private

def check_policy_numbers
  if params[:policy_numbers].nil? && params[:commission_dates].nil?
    redirect_to :action => 'find'
    false # return false so the action isn't called after this
  end
end

def numbers_and_dates_notice(numbers, dates)
  [policy_numbers_notice(numbers), commission_dates_notice(dates)].compact.join(' and ')
end

def policy_numbers_notice(numbers)
  "policy numbers #{numbers.values.join(', ')}" unless numbers.nil?
end

def commission_dates_notice(dates)
  "commission on dates #{format_dates_hash(dates).join(', ')}" unless dates.nil?
end

def format_dates_hash(dates)
  dates.collect { |d| "#{d[:mon]}/#{d[:year]}" } unless dates.nil?
end


You may want to move some of these private controller methods into a module or something so it doesn't clutter up the controller too much.

Don't forget about the model, we left it a mess:

# in Proposal
def self.update_policy_numbers(numbers_h, dates_h)
  id_array = Array.new
  id_array.concat(numbers_h.keys) unless numbers_h.nil?
  id_array.concat(dates_h.keys) unless dates_h.nil?
  update_proposals = Proposal.find(id_array)
  update_proposals.each { |p|
    attributes_h = Hash.new
    attributes_h[:policy_number] = numbers_h["#{p.id}"] if !numbers_h.nil? && numbers_h.key?("#{p.id}")
    attributes_h[:commission_on] = Date.new(dates_h["#{p.id}"][:year].to_i, dates_h["#{p.id}"][:mon].to_i, dates_h["#{p.id}"][:day].to_i) if !dates_h.nil? && dates_h.key?("#{p.id}")
    logger.debug "The attributes " + attributes_h.to_s
    p.update_attributes(attributes_h)
  }
end

I'm thinking we can move the stuff in the loop into an instance method:

def self.update_policy_numbers(numbers, dates)
  # default both to a hash so we don't have to worry about the nil possibility
  numbers ||= {}
  dates ||= {}
  Proposal.find(numbers.keys + dates.keys).each do |p|
    p.update_policy_number(numbers[p.id.to_s], dates[p.id.to_s])
  end
end

def update_policy_number(number, date)
  attributes = {} # I think this local variable is acceptable as it keeps the code simple and clean
  attributes[:policy_number] = number unless number.nil?
  attributes[:commission_on] = Date.new(date[:year].to_i, date[:mon].to_i, date[:day].to_i) unless date.nil?
  update_attributes(attributes)
end


Hopefully this post is as useful as it is long. wink

You may want to consider changing how the parameters are sent to this action. See my tutorials on creating and editing multiple models in one form for some ideas.

Railscasts - Free Ruby on Rails Screencasts

Re: Writing cleaner Ruby code

Hey Ryan, thanks again for your great response! I've used your new methods and moved 5 private methods of the controller in a module called ProposalsControllerHelper. I've placed it in the controllers directory.

I've gone through your tutorials and make some changes in the view and basically use the fields_for tag and the attributes method. So like in your tutorial, I can validate the whole list op proposals before actually saving each one. The only problem with this approach is that I that changing those two attributes causes a method change_state to be executed. So the new state is already set and added to the history of staes before the proposal instance is saved. :-(

  def policy_number=(new_policy_number)
    current_policy_number = self[:policy_number]
    self[:policy_number] = new_policy_number
    change_state(Policy.new) if current_policy_number.blank?
  end
 
  def commission_on=(new_commission_on)
    current_commission_on = self[:commission_on]
    self[:commission_on] = new_commission_on
    change_state(Commission.new) if current_commission_on.nil?
  end

  private
    def change_state(state)
      logger.debug 'change_state ' + state.to_s
      Proposal.transaction(self) do
        self.state = state
        self.states << state
      end
    end


There also a little mistake in your code above. It should be dates.values in stead of dates (collect can only be applied to an array):

Thanks again Ryan for your refactoring my messy code!

Mark

  def commission_dates_notice(dates)
    "commission on dates #{format_dates_hash(dates.values).join(', ')}" unless dates.nil?
  end

Re: Writing cleaner Ruby code

marknoten wrote:

The only problem with this approach is that I that changing those two attributes causes a method change_state to be executed. So the new state is already set and added to the history of staes before the proposal instance is saved. :-(

  def policy_number=(new_policy_number)
    current_policy_number = self[:policy_number]
    self[:policy_number] = new_policy_number
    change_state(Policy.new) if current_policy_number.blank?
  end
 
  def commission_on=(new_commission_on)
    current_commission_on = self[:commission_on]
    self[:commission_on] = new_commission_on
    change_state(Commission.new) if current_commission_on.nil?
  end

  private
    def change_state(state)
      logger.debug 'change_state ' + state.to_s
      Proposal.transaction(self) do
        self.state = state
        self.states << state
      end
    end

Is the state actually being saved in the change_state method? I imagine since the Proposal has not been saved, everything is still in memory and everything is saved once the proposal gets saved. I might be wrong though.

On setting the commission_on/policy_number, you may want to store the previous value in an instance variable and call change_state in a before/after_save callback. You can have a check in here to see what state to change to based off of the previous instance variables. This doesn't feel very clean, but it's difficult to see anything better without understanding the big picture.

Railscasts - Free Ruby on Rails Screencasts