1
\$\begingroup\$

i have 3 method which do almost the same thing,they different models to achieve the same goal.I did small refactoring but its still not looks like ruby,can we improve it? Thanks in advance.

after:

def get_list(id,list_name,attribute_id,attribute,locale_string)    
  map={ "resentact" => ResenAct }, "stageact" => StageAct , "payact" => PayAct" }
  @temp=""
  @list=map[list_name.downcase].find(:all, :conditions => {'id' => "#      {id}".chop_space})
   @list.each do |item|
      @temp = @temp + get_data(item.attribute_id,item) + " " + item.attribute + " " +   locale_string+ ", " 
   end
   return  @temp.pop(2) if @temp.length > 1
  end

method call : get_list(id,payact,payact_id,date,"some_string")

before:

 def get_resentact_list(id)    
 @temp=""
  @resentacts=ResentAct.find(:all, :conditions => {'product_id' => "#{id}".chop_space})
   @resentacts.each do |item|
      @temp = @temp + get_data(item.shop_id,"resentact") + " " + item.cost.to_s + " " + "string1" + ", " 
   end
   @temp = @temp[0,@temp.length-2] if @temp.length > 1
   return @temp

end

 def get_stageact_list(id)     
  @temp=""

    @stageacts=StageAct.find(:all, :conditions => {'product_id' => "#{id}".chop_space})
    @stageacts.each do |item|
       @temp = @temp + "string1" + ": " + get_data(item.type_id,"type") + ", " + "str2" + ": " + item.summ + " " + "str3" + ", "
    end
    @temp = @temp[0,@temp.length-2] if @temp.length > 1
    return @temp
 end 


def get_payact_list(id)  
  @temp=""

   @payacts=PayAct.find(:all, :conditions => {'product_id' => "#{id}".chop_space})
      @payacts.each do |item|
         @temp = @temp + "str1" + ": " + item.date + ", " + "str2" + ": " + item.summ + " " + "str3" + ", "
      end
      @temp = @temp[0,@temp.length-2] if @temp.length > 1
      return @temp
  end
\$\endgroup\$
  • 1
    \$\begingroup\$ 1. please re-format your code. 2. please provide test code. \$\endgroup\$ – Siwei Shen 申思维 Mar 19 '12 at 6:06
2
\$\begingroup\$
def get_payact_list(id)  
  ## Dont use class instance variables ( prefixed with @ ) for method scoped variables
  @temp="" # temp = ""
  ## In this case, you dont need the temporary variable, but lets keep it, but turn it into an array.
  # temp = [] 

  ## No need for the temp variable here, and still using class instance variables.
    @payacts=PayAct.find(:all, :conditions => {'product_id' => "#{id}".chop_space})
    @payacts.each do |item|
  # PayAct.find(:all, :conditions => {'product_id' => "#{id}".chop_space}).each do |item|

      @temp = @temp + "str1" + ": " + item.date + ", " + "str2" + ": " + item.summ + " " + "str3" + ", "
      ## Lets simply push each item onto the array.
      ## While we are at it, we remove the trailing comma string. 
      # temp << "str1" + ": " + item.date + ", " + "str2" + ": " + item.summ + " " + "str3"

    end
    # No need to mangle the end of the string, as we can simply join the temp array with a comma, which will not add to the end.
    @temp = @temp[0,@temp.length-2] if @temp.length > 1
    # temp.join ", "

    # no need for an explicit return. Ruby will return the value of the last expression.
  return @temp
end

Complete example, without changing too much.

def get_payact_list(id)  
  temp = [] 
  PayAct.find(:all, :conditions => {'product_id' => "#{id}".chop_space}).each do |item|
    temp << "str1" + ": " + item.date + ", " + "str2" + ": " + item.summ + " " + "str3"
  end
  temp.join ", " # is implicitly returned.
end

We can do away with the temp variable by using collect() instead of each() and pushing to an array.

def get_payact_list(id)  
  list = PayAct.find(:all, :conditions => {'product_id' => "#{id}".chop_space}).collect { |item|

    "str1" + ": " + item.date + ", " + "str2" + ": " + item.summ + " " + "str3" 
  } 
  list.join ", "
end  

Lets go the whole way, here are our new methods

This still looks and feels like duplication, but without a bit more metaprogramming than I want to go into right now, it will do. At least the separate concerns are factored out.

  def get_payact_list id 
    format_act_list PayAct.find_by_product_id(id).collect { |item| 
      [item.date, item.summ]
    }
  end

  # Note: the get_data method should be encapsulated on the item, leaving as is for now.
  def get_stageact_list id 
    format_act_list StageAct.find_by_product_id(id).collect { |item| 
      [ get_data(item.type_id,"type"), item.summ ] 
    }
  end

  def get_resentact_list id
     format_act_list ResentAct.find_by_product_id(id).collect { |item| 
       [ get_data(item.shop_id,"resentact"), item.cost ]
     }
  end


  def format_act_list list
    list.collect { |item| format_tuple(item) }.join ', '
  end

  # Takes an array and correctly formats the output
  # Ideally this would live in a presenter or a view. 
  def format_tuple tuple 
    "str1: %s, str2: %s str3" % [ tuple[0],  tuple[1] ]
  end

And this is the supporting code.

class Act
  # I've moved this common finder method up the (fake) chain of inheritance I invented.
  # It could also live in each of the child objects i.e ( PayAct StageAct ResentAct )
  def self.find_by_product_id (product_id)
    self.find(:all, :conditions => {'product_id' => "#{product_id}".chop_space}) 
  end
end

class PayAct < Act;end
class StageAct < Act; end
class ResentAct < Act; end
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.