重构这个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