I was reading some ruby one liners. The rescue style seems to make it easy to write short pieces of code, but it feels like I am forcing an error and then just ignoring it in some cases. I guess I am an older programmer and was never encouraged to write this sort of code, but do most people feel like this is good style ?
def receipt_available
quote_responses.count > 0 && rqstate.eql?(“submitted”) rescue false
end
obviously if code had alot of possible errors to recover from you wouldn’t do that
This is terrible style and is strictly forbidden in our codebase.
In the example below, probably the programmer thought that an exception always meant that quote_responses was nil instead of an array, and that nil should be treated as an empty array. That works fine… until another source of errors comes along. What if the interface for arrays changed such that count was no longer a valid method, so that this function always returned false? Instead of an obvious exception and a quick fix, the code would just return the wrong answer all the time. (We actually had this sort of problem when switching Rails versions.) Or maybe the name of the rqstate variable changed, and the “undefined variable” exception resulted in this code returning false - same problem.
Something like this is much better. Handle the particular situation that is causing exceptions, so that real problems will remain visible.
def receipt_available
(quote_responses || ).count > 0 && rqstate.eql?(“submitted”)
end
Save exception handling for cases where it is really needed. If there’s no way to avoid the exception, and you have a way to recover from the exception, then catch only that very specific type of exception. You can also have a high-level exception handler for the entire application that takes care of reporting uncaught exceptions so that they can be fixed.
rqstate is as follows, if there is not a quote request or the status field is null then we could assume
that the request was never submitted to the backend api. This was actually the first time I tried this approach but I am not sure I will keep it this way
def rqstate
self.quote_request.status rescue “unsubmitted”
end
I get the point that you are suppressing possible errors, however it seems you could avoid using rescue in many places where it is used.
What if the language made it so you could use that type of syntax and when an error occurred you could handle it someplace else, in effect defining some action for the rescue or is that possible ?
I first took notice of this:
myhash.keys.each do |key|
myhash[(key.to_sym rescue key) || key] = myhash.delete(key)
end
#try is very nice in this case, but avoid overdoing it. I have faced many codebases with tons of try methods chained and then you lose code readability.
def rqstate
quote_request.try(:status) || "unsubmitted"
end
#try is very nice in this case, but avoid overdoing it. I have faced many
codebases with tons of try methods chained and then you lose code
readability.
Personally I don't like try. I don't find it easy to intuitively
understand what it is doing, I always seem to have to think about the
code to work out what it is doing. Perhaps that is just my old brain
getting past it though. Of course the ? : syntax can easily become
opaque in more complex cases also.
I have been doing alot of contract work lately. I feel like I want to play with some of these one liners as at interviews I sometimes get asked such things but also I want to be flexible to the coding standards of any company I may work at.
A gotcha is that if quote_request is non nil, but doesn’t actually have a method called status then try will ignore the NoMethodError and just return nil ( try! on the other hand will raise in this case). Prior to rails 4, try behaved like try!