1 2 3 4
def self.get_budgeted_hours(charge, rollup) @value = find(:first, :conditions => ["charge_id = ? AND rollup_id = ?", charge, rollup]) @value ? @value.budgeted_hours : 0.to_s end
Refactorings
No refactoring yet !
Remco
March 3, 2008, March 03, 2008 22:41, permalink
1 2 3
def self.get_budgeted_hours(charge, rollup) find_by_charge_id_and_rollup_id(charge, rollup).budgeted_hours rescue '0' end
mwilliams
March 3, 2008, March 03, 2008 23:07, permalink
Doh, I don't know why I'm not using dynamic find by's here... The rescue was just what I needed, thanks!
Jeremy Weiskotten
March 5, 2008, March 05, 2008 01:10, permalink
The dynamic find_by is good, but I'm not a big fan of the rescue idiom -- not sure why, but probably because I don't like using errors to control logic. The previous refactoring also changes the behavior of the method -- it doesn't set the @value variable. If you need to set the variable, this should work.
1 2 3 4
def self.get_budgeted_hours(charge, rollup) @value = find_by_charge_id_and_rollup_id(charge, rollup) (@value && @value.budgeted_hours) || '0' end
rpheath
March 5, 2008, March 05, 2008 18:29, permalink
Since you're in a class method, you shouldn't need the instance variable at all, right? Maybe something like this...
1 2 3 4 5 6 7 8 9
def self.get_budgeted_hours(charge, rollup) find_by_charge_id_and_rollup_id(charge, rollup).budgeted_hours || 0 end # or if you do need the @value set... def self.get_budgeted_hours(charge, rollup) @value = find_by_charge_id_and_rollup_id(charge, rollup).budgeted_hours || 0 end
Jeremy Weiskotten
March 5, 2008, March 05, 2008 21:05, permalink
rpheath, that's not going to work if the find method doesn't find it, and returns nil. It won't return 0, it will throw a NoMethodError because NilClass doesn't respond to #budgeted_hours.
Ryan Bates
March 5, 2008, March 05, 2008 21:42, permalink
How about extending Object with a method that does this. Similar to the "try" method shown here: http://ozmm.org/posts/try.html
1 2 3 4 5 6 7 8 9 10
class Object def try_or_zero(method_name) respond_to?(method_name) ? send(method_name) : 0 end end # in your model def self.get_budgeted_hours(charge, rollup) find_by_charge_id_and_rollup_id(charge, rollup).try_or_zero(:budgeted_hours) end
mwilliams
March 5, 2008, March 05, 2008 23:23, permalink
Ahh that's exactly the kind of DRY-ness I've been looking for Ryan! I'll drop that in first ting at work tomorrow... Great work on Railscasts as well, I really enjoy it and it's a fantastic contribution to the Rails community.
rpheath
March 6, 2008, March 06, 2008 18:30, permalink
Jeremy Weiskotten - Yeah, I realized that. Guess I jumped on it too quickly. Just a question, does your version not return: (true|false) || '0'?
Here's another option (http://ruby.tie-rack.org/53/a-better-try-chaining-methods-and-nil/):
1 2 3 4 5 6 7
class Object def try_or_zero(method) self.nil? ? 0 : send(method) end end # same as above...
Jeremy Weiskotten
March 6, 2008, March 06, 2008 18:43, permalink
rpheath, no it won't return true or false. (x && x.foo) will return nil if x is nil. (x && x.foo) || '0' will return '0' if x is nil. If x is not nil, it will return x.foo.
leethal
April 7, 2008, April 07, 2008 11:01, permalink
Object extension
1 2 3 4 5 6 7 8 9 10
class Object def try(method_name) respond_to?(method_name) ? send(method_name) : 0 end end # The method def self.budgeted_hours(charge, rollup) @budget_hours ||= (find_by_carhe_id_and_rollup_id(charge, rollup).try(:budget_hours) || "0") end
I've got a variety of methods in my model that are very similar to the one posted here. It however seems overkill and that it should be easily simplified with some Ruby idiom that I don't know.
The problem is simple, when the method is called, return a value if one exists, otherwise return "0".
Thoughts? I don't know what I'm overlooking but I needed something quick and this is what I came up with and just stuck with it. I'm at a point where things could use a good refactor.
Thanks!