1

I have written customised getter and setter methods for virtual attributes to convert decimals into integers for storage in a database. This is one of three virtual attributes (annual_fee_dollars) that get/sets from a real attributes (annual_fee) in the database:

def annual_fee_dollars
  @annual_fee_dollars || int_to_dec(annual_fee)
end

def annual_fee_dollars=(string)
  @annual_fee_dollars = string
  self.annual_fee = dec_to_int(string)
end

Instead of repeating all this code three times, does it make sense to / is it safe to / is it the 'Rails Way' to refactor the code like this:

def self.decimal_get_and_set(variable, suffix)
  eval (
    "def #{variable + suffix}
      @#{variable + suffix} || int_to_dec(self.#{variable})
    end
    def #{variable+suffix}=(string)
      @#{variable+suffix} = string
      self.#{variable} = dec_to_int(string)
    end")
end
self.decimal_get_and_set "annual_fee", "_dollars"
self.decimal_get_and_set "interest_purchase", "_percent"
self.decimal_get_and_set "interest_cash", "_percent"

Or is there a much cleaner way to build this type of functionality?

Apologies if this is a 'subjective question'. In part, all refactoring questions have some subjectivity to them, but I think this question still has a place on SO. Happy to be corrected on this.

Cheers!

Rob d'Apice
  • 2,416
  • 1
  • 19
  • 29

1 Answers1

3

I think your approach is fine, but I wouldn't suggest using eval, mainly because there is already a more appropriate ruby metaprogramming way to do this. Read up on the documentation for define_method and the object methods instance_variable_get and instance_variable_set.

Looks like what you want and you don't need to use eval yourself. I would probably suggest something like the following, but you're right - all refactoring questions are somewhat subjective by their very nature. Good luck!

{'annual_fee' => '_dollars', 'interest_purchase' => '_percent', 'interest_cash' => '_percent'}.each_pair do |variable, suffix|
  # Define getters
  define_method "#{variable+suffix}" do
    instance_variable_get("@#{variable+suffix}") || int_to_dec(send("#{variable}")
  end

  # Define setters
  define_method "#{variable+suffix}=" do
    ...
  end
end
Rob d'Apice
  • 2,416
  • 1
  • 19
  • 29
Brett Bender
  • 19,388
  • 2
  • 38
  • 46
  • Why not have a method which gets and sets a value given a property name, define 2 getters and setters for both the aliased methods to **curry** the defined method. I would do that rather than eval as I find that eval code can make stuff more confusing depending on how you use it. The code you showed `Brett` is in no way super complex, but for readability I feel this is heavy-handing the problem. – Dmitriy Likhten May 16 '11 at 17:34
  • thanks for the answer. This seems a lot cleaner - and easier to read - than the 'eval' solution. I wasn't aware of the various class/module methods you've used (instance_variable_get, et al). I'm still tempted to wrap your two 'define_methods' in a class method, then call it three times (once for each attribute) - rather than set up the 'each_pair' do loop. I can't explain why except that it'll be clearer when editing in the future. Thanks for your answer! – Rob d'Apice May 16 '11 at 23:30
  • @Dmitriy - I hadn't heard of the curry method. Having looked into it, I'm not sure I have the Ruby smarts to implement it well. Also, I'm running 1.8.7 - looks like curry wasn't introduced until 1.9.0? – Rob d'Apice May 16 '11 at 23:33
  • The getter method is slightly wrong - it's either fetching instance variable or (if it is nil) fetching int_to_dec of the instance variable. I've corrected it to fetch the underlying attribute using the send method – Rob d'Apice May 16 '11 at 23:49