重構這個Ruby和Rails代碼

Question

我有這樣的模式：

class Event < Registration 
    serialize :fields, Hash 
    Activities=['Annonce', 'Butiksaktivitet', 'Salgskonkurrence'] 

    CUSTOM_FIELDS=[:activity, :description, :date_from, :date_to, :budget_pieces, :budget_amount, :actual_pieces, :actual_amount] 
    attr_accessor *CUSTOM_FIELDS 

    before_save :gather_fields 
    after_find :distribute_fields 

    private 

    def gather_fields 
    self.fields={} 
    CUSTOM_FIELDS.each do |cf| 
     self.fields[cf]=eval("self.#{cf.to_s}") 
    end 
    end 

    def distribute_fields 
    unless self.fields.nil? 
     self.fields.each do |k,v| 
     eval("self.#{k.to_s}=v") 
     end 
    end 
    end 
end 

我有一種感覺，這是可以做到更短，更優雅。有人有想法嗎？

• 雅各

BTW。任何人都可以告訴我CUSTOM_FIELDS前的星號是幹什麼的？我知道它的方法定義（DEF foo的（*參數）），但不是在這裏......

Answer 1

好吧先關閉：從不10000000000.times { puts "ever" }使用eval當你不知道你在做什麼。這是Ruby世界的核彈，它可以在廣泛的區域發生反轉，導致整個代碼中的輻射中毒類似的症狀。不要。

考慮到這一點：

class Event < Registration 
    serialize :fields, Hash 
    Activities = ['Annonce', 'Butiksaktivitet', 'Salgskonkurrence'] 

    CUSTOM_FIELDS = [:activity, 
       :description, 
       :date_from, 
       :date_to, 
       :budget_pieces, 
       :budget_amount, 
       :actual_pieces, 
       :actual_amount] #1 
    attr_accessor *CUSTOM_FIELDS #2 

    before_save :gather_fields 
    after_find :distribute_fields 

    private 

    def gather_fields 
    CUSTOM_FIELDS.each do |cf| 
     self.fields[cf] = send(cf) #3 
    end 
    end 

    def distribute_fields 
    unless self.fields.empty? 
     self.fields.each do |k,v| 
     send("#{k.to_s}=", v) #3 
     end 
    end 
    end 
end 

現在對於一些注意事項：

1. 通過將每個自定義字段在其自己的行，可以增加代碼的可讀性。我不想滾動到行尾以讀取所有可能的自定義字段或添加我自己的字段。
2. 傳入attr_accessor的*CUSTOM_FIELDS使用了所謂的「splat操作符」。通過以這種方式調用它，CUSTOM_FIELDS數組的元素將作爲單個參數傳遞給attr_accessor方法，而不是作爲一個（數組本身）
3. 最後，我們使用send方法調用我們不知道的方法編程時的名字，而不是邪惡的eval。

除此之外，我找不到其他任何東西來重構此代碼。

Answer 2

你可以送話費代替兩個evals：

self.fields[cf] = self.send(cf.to_s) 


self.send("#{k}=", v) 

「＃{}」不一個to_s，所以你不需要k.to_s

活動，作爲一個常數，應該可能是ACTIVITIES。

Answer 3

對於星號*，看看這篇文章：What is the splat/unary/asterisk operator useful for?

Activities=['Annonce', 'Butiksaktivitet', 'Salgskonkurrence'] 

可以寫成：ACTIVITIES = %w(Annonce, Butiksaktivitet, Salgskonkurrence).freeze，因爲你要定義一個常量。

def distribute_fields 
    unless self.fields.empty? 
    self.fields.each do |k,v| 
     send("#{k.to_s}=", v) #3 
    end 
    end 
end 

可以寫成一個班輪：

def distribute_fields 
    self.fields.each { |k,v| send("#{k.to_s}=", v) } unless self.fields.empty? 
end 

瑞恩比格，給了一個很好的答案。

Answer 4

我同意以前的海報。另外，我可能會將gather_fields和distribute_fields方法移到父模型中，以避免在每個子模型中重複該代碼。

class Registration < ActiveRecord::Base 
    ... 

protected 
    def gather_fields(custom_fields) 
    custom_fields.each do |cf| 
     self.fields[cf] = send(cf) 
    end 
    end 

    def distribute_fields 
    unless self.fields.empty? 
     self.fields.each do |k,v| 
     send("#{k.to_s}=", v) 
     end 
    end 
    end 
end 

class Event < Registration 
    ... 

    before_save :gather_fields 
    after_find :distribute_fields 

private 
    def gather_fields(custom_fields = CUSTOM_FIELDS) 
    super 
    end 
end