Refactor this Ruby and Rails code

I have this model:

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

I have a feeling that this can be done shorter and more elegant. Does anyone have an idea?

• Jacob

BTW. Can anyone tell me what the asterisk in front of CUSTOM_FIELDS does? I know what it does 开发者_如何学Goin a method definition (def foo(*args)) but not here...

Alright first off: never 10000000000.times { puts "ever" } use eval when you don't know what you're doing. It is the nuclear bomb of the Ruby world in the way that it can wreak devestation across a wide area, causing similar symptoms to radiation poisoning throughout your code. Just don't.

With that in mind:

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

Now for some notes:

1. By putting each custom field on its own line, you increase code readability. I don't want to have to scroll to the end of the line to read all the possible custom fields or to add my own.
2. The *CUSTOM_FIELDS passed into attr_accessor uses what is referred to as the "splat operator". By calling it in this way, the elements of the CUSTOM_FIELDS array will be passed as individual arguments to the attr_accessor method rather than as one (the array itself)
3. Finally, we use the send method to call methods we don't know the names of during programming, rather than the evil eval.

Other than that, I cannot find anything else to refactor about this code.

I agree with previous posters. In addition I would probably move the gather_fields and distribute_fields methods to the parent model to avoid having to repeat the code in every child model.

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

You can replace the two evals with send calls:

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


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

"#{}" does a to_s, so you don't need k.to_s

Activities, being a constant, should probably be ACTIVITIES.

For that asterisk *, check out this post: What is the splat/unary/asterisk operator useful for?

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

can be written: ACTIVITIES = %w(Annonce, Butiksaktivitet, Salgskonkurrence).freeze since you are defining a constant.

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

can be written as a one liner:

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

Ryan Bigg, gave a good answer.