Reputation: 5580
I think that I'm probably not writing lazily instantiated methods/attributes in the most ruby way. Take this method as an example:
def tax
@tax ||= Proc.new do
if flat_tax > commission_plan.tax_max
return commission_plan.tax_max
end if commission_plan.tax_max
if flat_tax < commission_plan.tax_min
return commission_plan.tax_min
end if commission_plan.tax_min
flat_tax
end.call
end
Is there a more ruby-like way to refactor this method?
Upvotes: 2
Views: 535
Reputation: 30495
If you don't want to add a dependency to an external library, you can easily add your own "memoize" helper. Something along the lines of:
class Class
def memoize(method)
original_method = instance_method(method)
instance_var = "@__#{method}__".to_sym
define_method(method) do |*a,&b|
cached = instance_variable_get(instance_var)
unless cached
cached = old_method.bind(self).call(*a,&b)
instance_variable_set(instance_var, cached)
end
cached
end
end
end
And then usage would be like:
def tax
# expensive calculation here
end
memoize :tax
If you don't like this memoize
interface, you can change it to whatever you like. This is Ruby, baby! The language which you can twist, bend, and stretch like bubble gum. Maybe an interface like this would be nice:
def_memoized :tax do
# expensive calculation here
end
I like to put my per-project extensions to core Ruby in a file called lib/core_extensions.rb
. This is the kind of thing that would go in there.
Upvotes: 2
Reputation: 31632
def tax
@tax ||= calc_tax
end
private
def calc_tax
min, max = commission_plan.tax_min, commission_plan.tax_max
if (min..max).include? flat_tax
flat_tax
else
flat_tax > max ? max : min
end
end
Upvotes: 6
Reputation: 2203
What you are asking about it called Memoization. As Yuri suggests, it is awkward that you are using a Proc for this.
Here is my quick refactoring. I'd probably still refactor this further... but it is a simple refactoring which is more Ruby-ish.
def tax
@tax ||= calculate_tax
end
def calculate_tax
if commission_plan.tax_max && flat_tax > commission_plan.tax_max
commission_plan.tax_max
elsif commission_plan.tax_min && flat_tax < commission_plan.tax_min
commission_plan.tax_min
else
flat_tax
end
end
Also, if you don't mind including some external dependencies, check out ActiveSupport::Memoizable. And here is an article that talks about memoization.
Upvotes: 2
Reputation: 2245
I don't understand why you are creating this anonymous function. It's... redundant. This is a much better and cleaner code:
def tax
return commission_plan.tax_max if commission_plan.tax_max &&
flat_tax > commission_plan.tax_max
return commission_plan.tax_min if commission_plan.tax_min &&
flat_tax > commission_plan.tax_min
return flat_tax
end
There are other ways of implementing it, but this is a great improving compared to what you have there.
Upvotes: 1