If there is an error, it should be handled as an error -- not hidden away. If I put an object in, but get a totally unrelated object back, that is an unpleasant surprise. I'd rather just be told that an exception happened so I can either deal with it or have it bubble up so we will find out that we forgot to handle it.
If a method returns an unexpected object, and we haven't taught all the rest of our code to check whether it received an imposter, it's too easy to accidentally have our code treat it like a real object. On Oct 6, 2010 7:59 AM, "Markus Roberts" <[email protected]> wrote: >>> diff --git a/app/models/report.rb b/app/models/report.rb >>> index 04c81b8..46e6728 100644 >>> --- a/app/models/report.rb >>> +++ b/app/models/report.rb >> [...] >>> @@ -24,7 +24,8 @@ class Report < ActiveRecord::Base >>> end >>> >>> def report >>> - rep = read_attribute(:report) >>> + # Returning a blank report will let us fail gracefully for a bad report >>> + rep = read_attribute(:report) rescue Puppet::Transaction::Report.new >>> rep.extend(ReportExtensions) unless rep.nil? or rep.is_a?(ReportExtensions) >>> rep >>> end >> This approach is problematic because it doesn't check the object it's >> using, what type it is or whether it's valid. For example, if I set >> the report attribute to a blank string or anything other than a >> Puppet::Transaction::Report, this method and/or code that expected it >> to work will fail in confusing ways. Also, instantiating a blank >> placeholder Puppet::Transaction::Report object is dangerous because >> then other code may fail in surprising ways because it may mistake it >> for a valid report. >> >> It may be best for this #report method to either throw a general >> TypeError or a custom InvalidReportFormatError if it can't get a valid >> Puppet::Transaction::Report object and add exception handlers to code >> relying on this method. For example, the >> Report#report_contains_metrics method could catch this error and >> return false, while the Report#set_attributes could catch this and set >> the denormalized values for #success, #time and #host to nil because >> it has nothing to set them to. >> >> An example of the trouble with this patch's approach is that >> `Report.new.valid?` should return false, but instead throws a >> "NoMethodError: undefined method `failed?' for nil:NilClass" because >> the Report#set_attributes method assumes that the #report is always a >> valid Puppet::Transaction::Report. > > I had trouble working through exactly what your objection was here > (perhaps a low-coffee problem). Are you basically saying that 1) > rescuing errors is not sufficient because it doesn't handle cases > where something other than a Puppet::Transaction::Report is returned > from read_attribute and 2) minting a new Puppet::Transaction::Report > won't fail gracefully because...and this part I'm still not clear on. > > -- Markus > ----------------------------------------------------------- > The power of accurate observation is > commonly called cynicism by those > who have not got it. ~George Bernard Shaw > ------------------------------------------------------------ > > -- > You received this message because you are subscribed to the Google Groups "Puppet Developers" group. > To post to this group, send email to [email protected]. > To unsubscribe from this group, send email to [email protected]<puppet-dev%[email protected]> . > For more options, visit this group at http://groups.google.com/group/puppet-dev?hl=en. > -- You received this message because you are subscribed to the Google Groups "Puppet Developers" group. To post to this group, send email to [email protected]. To unsubscribe from this group, send email to [email protected]. For more options, visit this group at http://groups.google.com/group/puppet-dev?hl=en.
