Daily WTF: NilClass#method_missing
With great power comes great responsibility.
I’ve been leading Ruby and Rails training over the last couple weeks for a company in Brisbane, Australia, Xerox in El Segundo, CA, and yellowpages.com in Glendale, CA. Visiting companies is always so interesting because you get to see how different people work together and how they go about solving problems. You also get to see some really interesting code.
I was helping one of the companies walk through some code they inherited from an acquisition and came across something similar to this wonderful snippet of code:
class NilClass def method_missing(method, *args) raise(NoMethodError, "undefined method '#{method}' for nil") rescue => e CustomLogger.error(e) nil end end
WTF? They’re overriding #method_missing on nil to raise an exception, they then proceed to rescue it, log the error, and return nil!
I’m guessing they had some buggy code that was calling a method on nil, causing a NoMethodError to be raised. And instead of fixing the code, they just decided to change it so that you could call methods on nil and it would just silently fail.
>> nil.foo.bar.baz => nil
It took me a while to figure out why they were raising an error and then rescuing it, but I’m guessing it’s so they have access to a stack trace in the logger.
I’ll give them credit for being creative, but whoever wrote that piece of code doesn’t deserve to be using Ruby. Yes, Ruby’s open classes are powerful, but that’s just ridiculous.
12 Comments
Apart from the fact the strack trace is always available via
caller.The strack trace is always available via
calleranyway.I completely agree. I found this in LovdByLess (which has been getting a lot of attention lately):
http://github.com/stevenbristol/lovd-by-less/tree/master/vendor/plugins/less_monkey_patching/lib/nil.rb
WTF?
Spend enough time with a complex Rails app, and you’ll understand why this happens.
This is often a fairly reasonable thing to do. Log the event, return nil to (usually) the view rendering code so that the app doesn’t 500 for your users, and follow up on the log messages and fix yr damn code. :)
andrew:
I’ve spent every day of the last 2 years working on Rails apps. I don’t know that I’ve worked on a project yet that I wouldn’t classify as “complex”.
Why stop at overriding
#method_missingonNilClass? Why not just override it onObjectso that any object responds to any method? That would make sure that you never get an error message!If your code is hitting a condition where it would raise an error, then it’s probably not doing what the user expects anyway. Why pretend like nothing is wrong? Your users would appreciate if you quit wasting there time and just acknowledged that there is an error and you’re working on fixing it.
Overriding
#method_missingonNilClassin this manner is simply ridiculous. It is the symptom of poor programming.Nah, you’re missing the use case: sometimes Rails views will 500 due to “impossible” data conditions, far away from the bug (and maybe long after the bug has been fixed). Unless you have an ultra-rich and constantly-running data integrity checker, these unexpected conditions can result in error pages for users when you try to display the username of the owner of a comment on a video belonging to a group owned by a user that might have been deleted.
Careful and judicious use of NilClass#method_missing (with good logging and followup) can save a ton of work. An alternative would be to nil protect everything in views (and then what would you do anyway? Typically return nil or skip the record entirely, which encourages the bug or data corruption to go unnoticed for far longer than reasonable use of method_missing would).
With great power comes great responsibility. Sure. I’d argue that this is a perfect example of a time when, if you’re being greatly responsible, you can use the great power for great benefit.
Oh.. and the raise/rescue is probably just a cheap way to hook an existing ExceptionNotifier method, yet still allow the page to render without error.
I think the implementation is meant to mimic how Objective-C works with nil/null. The cool thing about the core of the idea (returning nil on nil.whatever) is pretty neat in that you dont have to do nil checking. If you have a decision that includes nil.whatever it will always return false instead of having to do my_nil_object && my_nil_object.method_i_intended_to_call.
Its neat and works in objective c, so…
I agree with Josh – not having to do nil checking seems like it would make code a lot neater. E.g., if I want to check an attribute on some object, I could just do:
Model.find(:first, :condtions => [ “some conditions” ]).attribute
and not have to worry about whether the find works or not; if it’s nil, I’ll get nil back anyway. If I really want to do something else, I can always check nil by hand. I’m not losing anything this way, so long as I can still trace my errors.
+1 to Josh.
This is how Obj-C works, and it actually works out quite neatly if you accept it as a part of the language and code to it.
Part of the power of Ruby is that you can bend the semantics of the language. Yes, dangerous. No, not always bad.
Watch out for snide judgements like “doesn’t deserve to be using Ruby.” That may be ignorance speaking.
Paul/Josh: I love this about Obj-C, but the difference is that every single Obj-C library expects this to be the case. Rubyists expect nil to act like nil.
Changing low level things like this make it very difficult to figure out when things go wrong.
After having written too many times
I just did
and you may call it un-ruby-like or whatever, but it saves me much trouble and code.
Post a Comment