options.each_pair do |attribute, default_value|
define_method attribute do
eval "@ ||= "
end
end
end
attr_with_default :emails => "[]", :employee_number => "EmployeeNumberGenerator.next"
end
becomes
options.each_pair do |attribute, default_value|
eval "def
@ ||=
end"
end
end
attr_with_default :emails => "[]", :employee_number => "EmployeeNumberGenerator.next"
end
Motivation
premature optimization is the root of all evil -- Knuth, DonaldI'll never advocate for premature optimization, but this refactoring can be helpful when you determine that eval is a source of performance pain. The Kernel#eval method can be the right solution in some cases; but it is almost always more expensive (in terms of performance) than it's alternatives. In the cases where eval is necessary, it's often better to move an eval call from run-time to parse-time.
Mechanics
- Expand the scope of the string being eval'd.
- Test
The following Person class uses eval to define the logic the readers rely upon for returning a default value if no value has previously been set.
options.each_pair do |attribute, default_value|
define_method attribute do
eval "@ ||= "
end
end
end
attr_with_default :emails => "[]", :employee_number => "EmployeeNumberGenerator.next"
end
The above example executes without issue, but it relies upon eval each time a reader is called. If multiple calls to eval are determined to be problematic the solution is to expand the eval to include defining the method itself.
options.each_pair do |attribute, default_value|
eval "def
@ ||=
end"
end
end
attr_with_default :emails => "[]", :employee_number => "EmployeeNumberGenerator.next"
end
At the risk of being a lot more verbose, in the example you've given you could use:
ReplyDeleteivar = "@#{attribute}"
instance_variable_set(ivar, default_value) unless instance_variable_get(ivar)
instance_variable_get(ivar)
This would avoid using a string eval at all. My first inclination would be to use this, even without looking at performance numbers. I tend to avoid string eval whenever I can see an easy way out. What do you think? Should I not be so "scared" of eval in simple circumstances like this?
In theory, I expect the database, network, etc to be the bottlenecks, not whether or not I use eval.
ReplyDeleteIn practice, I also avoid string eval, and eval in general if I can. I would also use instance_variable_set or instance_variable_get when possible.
Pragmatically, we should probably both be less worried about the performance of eval and focus on writing the cleanest code possible.
Also, there are times when you simply need to string eval. In my example, I want the default value to be anything that can be specified as a string. For example, you may not want EmployeeNumberGenerator to execute the next method until the employee_number method is called. In which case you'll need to pass in either a block or a string and eval when the reader is called.
And, thanks for the comment. =)
ReplyDeleteCheers, Jay