refactor ruby method

Question

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

• Siwei: 1. please re-format your code. 2. please provide test code.

Answer 1

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