Refactor this Ruby and Rails code

Question

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 in a method definition (def foo(*args)) but not here...

I think this question fits better in codereview (codereview.stackexchange.com - the beta is now active!) — ecoologic
– ecoologic, Commented Jan 20, 2011 at 23:10

Ryan Bigg · Accepted Answer · 2011-01-20 10:57:33Z

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:

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.
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)
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.

Thanks so much for your solution. I promise that I won't use eval ever again (almost :-)).

DanneManne · Accepted Answer · 2011-01-20 11:16:20Z

1

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

answered Jan 20, 2011 at 11:16

DanneManne

21.2k5 gold badges60 silver badges58 bronze badges

2 Comments

jriff Over a year ago

Great - you are so right! I have implemented that - I am wondering. Can I do something like send("#{k.to_s}=", v) if attr_defined?(k). That way the code won't blow up if the attribute isn't defined for the class...

DanneManne Over a year ago

Yes, either that or you could send custom_fields to that method as well and loop through them instead of self.fields

Satya · Accepted Answer · 2011-01-20 10:55:34Z

0

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.

answered Jan 20, 2011 at 10:55

Satya

4,49824 silver badges30 bronze badges

Comments

Mirko · Accepted Answer · 2011-01-20 11:14:23Z

0

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.

edited Jan 20, 2011 at 11:14

answered Jan 20, 2011 at 10:59

Mirko

5,3062 gold badges40 silver badges34 bronze badges

Collectives™ on Stack Overflow

Refactor this Ruby and Rails code

4 Answers 4

1 Comment

2 Comments

Comments

Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

4 Answers 4

1 Comment

2 Comments

Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Related