Rails core - Apr 2005 - What are people''s thoughts on defensive coding?

The main users list quite often has questions like, "Why do I get
NoMethodError when calling such-and-such", they''re almost always
because something is nil.  While it does bulk out the code, I''m quite
fond of explicit nil tests for critical parameters.  Take
collection_select,  if the collection passed in is nil, the users get
an error "undefined method inject for nil:NilClass", 
wouldn''t it be
nicer if the error was something along the lines of "Nil collection
passed to collection_select, Please ensure the collection was
initialized".

-- 
Cheers

Koz

On Wed, Apr 27, 2005 at 09:53:08AM +1200, Michael Koziarski
wrote:
> The main users list quite often has questions like, "Why do I get
> NoMethodError when calling such-and-such", they''re almost
always
> because something is nil.  While it does bulk out the code, I''m
quite
> fond of explicit nil tests for critical parameters.  Take
> collection_select,  if the collection passed in is nil, the users get
> an error "undefined method inject for nil:NilClass", 
wouldn''t it be
> nicer if the error was something along the lines of "Nil collection
> passed to collection_select, Please ensure the collection was
> initialized".
I think helpful error messages are a good thing (obviously) but adding code
to take special cases into account can often clutter up what would otherwise
be short and simple. Rather than put checks all over the place, it seems like
it might be cleaner and sufficient to define a more useful method_missing for
NilClass while in development mode, since it does often indeed seem to be the
case that beginners are trying to call something on nil and not realizing it.
In the case of the above example this approach wouldn''t be much help
though.
Maybe a nice of implementation of Design by Contract for Ruby would allow for
"defensive coding" in a more organized / less cluttered way, but my
$0.02 is
that sticking 

  raise "Some more helpful error message" if blah.nil? 

all over the place would be unseemly. Perhaps for the most cryptic cases,
though, it would be worth it. 

marcel
-- 
Marcel Molina Jr. <marcel@vernix.org>

On 4/27/05, Marcel Molina Jr. <marcel@vernix.org>
wrote:
> On Wed, Apr 27, 2005 at 09:53:08AM +1200, Michael Koziarski wrote:
> > The main users list quite often has questions like, "Why do I get
> > NoMethodError when calling such-and-such", they''re
almost always
> > because something is nil.  While it does bulk out the code,
I''m quite
> > fond of explicit nil tests for critical parameters.  Take
> > collection_select,  if the collection passed in is nil, the users get
> > an error "undefined method inject for nil:NilClass", 
wouldn''t it be
> > nicer if the error was something along the lines of "Nil
collection
> > passed to collection_select, Please ensure the collection was
> > initialized".
> 
> I think helpful error messages are a good thing (obviously) but adding code
> to take special cases into account can often clutter up what would
otherwise
> be short and simple. Rather than put checks all over the place, it seems
like
> it might be cleaner and sufficient to define a more useful method_missing
for
> NilClass while in development mode, since it does often indeed seem to be
the
> case that beginners are trying to call something on nil and not realizing
it.
> In the case of the above example this approach wouldn''t be much
help though.
> Maybe a nice of implementation of Design by Contract for Ruby would allow
for
> "defensive coding" in a more organized / less cluttered way, but
my $0.02 is
> that sticking
> 
>   raise "Some more helpful error message" if blah.nil?
> 
> all over the place would be unseemly. Perhaps for the most cryptic cases,
> though, it would be worth it.
I''d agree that it''s unseemly,  though it only affects ~10 of
us,
whereas the better message help the, hopefully, quadrizillions of
rails users that are in the pipeline.  It''s a hit I''d be
willing to
take?

Plus, I''d disagree that this is only useful in development,
it''s
*more* useful in production as you get a detailed error message rather
than ''NoMethodError''.   Dead Programs Tell No Lies?

> marcel
> --
> Marcel Molina Jr. <marcel@vernix.org>
> _______________________________________________
> Rails-core mailing list
> Rails-core@lists.rubyonrails.org
> http://lists.rubyonrails.org/mailman/listinfo/rails-core
> 

-- 
Cheers

Koz

Marcel Molina Jr. wrote:
>> Rather than put checks all over the place, it seems like
>> it might be cleaner and sufficient to define a more useful
method_missing for
>> NilClass while in development mode, since it does often indeed seem
to be the
>> case that beginners are trying to call something on nil and not
realizing it.


That''s a neat idea, Marcel.  Try throwing this in
config/environments/development.rb and see whether it chaps your hide.

class NilClass
  alias_method :id_without_warning, :id

  def id
    method_missing :id
    id_without_warning
  end

  private
    def method_missing(method, *args, &block)
      RAILS_DEFAULT_LOGGER.warn <<end_warning

!! NIL ALERT !!

#{method} called on a nil object in
  #{caller.join("\n  ")}
end_warning
    end
end

jeremy

On 4/27/05, Jeremy Kemper <jeremy@bitsweat.net>
wrote:
> Marcel Molina Jr. wrote:
> >> Rather than put checks all over the place, it seems like
> >> it might be cleaner and sufficient to define a more useful
> method_missing for
> >> NilClass while in development mode, since it does often indeed
seem
> to be the
> >> case that beginners are trying to call something on nil and not
> realizing it.
> 
> That''s a neat idea, Marcel.  Try throwing this in
> config/environments/development.rb and see whether it chaps your hide.
> 
> class NilClass
>   alias_method :id_without_warning, :id
> 
>   def id
>     method_missing :id
>     id_without_warning
>   end
> 
>   private
>     def method_missing(method, *args, &block)
>       RAILS_DEFAULT_LOGGER.warn <<end_warning
> 
> !! NIL ALERT !!
> 
> #{method} called on a nil object in
>   #{caller.join("\n  ")}
> end_warning
>     end
> end
This has the effect of cleaning up the error message which is an
improvement,  but processing will continue which will probably confuse
the user more.  In the case that someone passes a nil collection to
collection_select they''ll just get an empty page.

Plus,  we still haven''t told them what argument isn''t allowed
to be
nil.   I''m not suggesting starting each and every method in the entire
codebase with 20 lines of raise "" if blah.nil?.  But I do believe in
failing fast and telling the user what *they* did wrong (passing nil),
 not what we did wrong (calling methods on it).
> jeremy
> _______________________________________________
> Rails-core mailing list
> Rails-core@lists.rubyonrails.org
> http://lists.rubyonrails.org/mailman/listinfo/rails-core
> 

-- 
Cheers

Koz

---------- Forwarded message ----------
From: Nicholas Seckar <nseckar@gmail.com>
Date: Apr 27, 2005 2:55 AM
Subject: Re: [Rails-core] What are people''s thoughts on defensive
coding?
To: michael@koziarski.com


On 4/27/05, Michael Koziarski <koziarski@gmail.com>
wrote:
> This has the effect of cleaning up the error message which is an
> improvement,  but processing will continue which will probably confuse
> the user more.  In the case that someone passes a nil collection to
> collection_select they''ll just get an empty page.
Another choice might be to provide better error messages. For example,
nil.each { ... } could raise an exception such as "A nil object was
used where a collection was expected", nil.save, nil.valid?, etc =>
"A
model object was expected but nil was found."

The only bad part about the current setup is that the error messages
confuse those new to Ruby. If the issue is that the error messages are
slightly misleading, we should remedy those messages and leave the
rest as is.

On 4/27/05, Nicholas Seckar <nseckar@gmail.com>
wrote:
> ---------- Forwarded message ----------
> From: Nicholas Seckar <nseckar@gmail.com>
> Date: Apr 27, 2005 2:55 AM
> Subject: Re: [Rails-core] What are people''s thoughts on defensive
coding?
> To: michael@koziarski.com
> 
> On 4/27/05, Michael Koziarski <koziarski@gmail.com> wrote:
> > This has the effect of cleaning up the error message which is an
> > improvement,  but processing will continue which will probably confuse
> > the user more.  In the case that someone passes a nil collection to
> > collection_select they''ll just get an empty page.
> 
> Another choice might be to provide better error messages. For example,
> nil.each { ... } could raise an exception such as "A nil object was
> used where a collection was expected", nil.save, nil.valid?, etc =>
"A
> model object was expected but nil was found."
I like the idea of that, It''s better than the current setup as users
get a relevant error message,  but it doesn''t require too much stuff
to be scattered about the code.  Seems like a good balance really.   I
could whip this up and create a ticket,  unless someone else is
already onto it?
 
> The only bad part about the current setup is that the error messages
> confuse those new to Ruby. If the issue is that the error messages are
> slightly misleading, we should remedy those messages and leave the
> rest as is.
> _______________________________________________
> Rails-core mailing list
> Rails-core@lists.rubyonrails.org
> http://lists.rubyonrails.org/mailman/listinfo/rails-core
> 

-- 
Cheers

Koz

Michael Koziarski wrote:
> On 4/27/05, Nicholas Seckar <nseckar@gmail.com> wrote:
>>On 4/27/05, Michael Koziarski <koziarski@gmail.com> wrote:
>>
>>>This has the effect of cleaning up the error message which is an
>>>improvement,  but processing will continue which will probably
confuse
>>>the user more.  In the case that someone passes a nil collection to
>>>collection_select they''ll just get an empty page.
Then raise an exception.  Since it prints out the call stack, it''s easy
to identify where the nil object was passed.  This doesn''t hold your
hand as tightly as it could, but it does give a very simple,
non-intrusive way of warning the user to watch his bad behavior.

I do not want nil checks all over Rails.  Next it''ll be type checks.

>>Another choice might be to provide better error messages. For example,
>>nil.each { ... } could raise an exception such as "A nil object was
>>used where a collection was expected", nil.save, nil.valid?, etc
=> "A
>>model object was expected but nil was found."
> 
> I like the idea of that, It''s better than the current setup as
users
> get a relevant error message,  but it doesn''t require too much
stuff
> to be scattered about the code.  Seems like a good balance really.   I
> could whip this up and create a ticket,  unless someone else is
> already onto it?
Please do.  Since this is a development aid, I''d like to be able to
easily add and remove it.  Something like require ''whiny_nil''
in
development (and perhaps test) environments.  Maybe
''smartass_nil'' ;)

jeremy

ps -- still wishing replies would at least CC the list automatically!

> Please do.  Since this is a development aid, I''d like to be able
to
> easily add and remove it.  Something like require
''whiny_nil'' in
> development (and perhaps test) environments.  Maybe
''smartass_nil'' ;)

I opted for whiny_nil.  The patch is at

http://dev.rubyonrails.org/ticket/1209 
> jeremy
> 
> ps -- still wishing replies would at least CC the list automatically!
> 
They will now, allegedly

-- 
Cheers

Koz

Michael Koziarski wrote:
>>Please do.  Since this is a development aid, I''d like to be
able to
>>easily add and remove it.  Something like require
''whiny_nil'' in
>>development (and perhaps test) environments.  Maybe
''smartass_nil'' ;)
>
> I opted for whiny_nil.  The patch is at
>    http://dev.rubyonrails.org/ticket/1209 
I posted a comment.
> They will now, allegedly
They do, certainly!  Thank you.

jeremy

On 4/28/05, Jeremy Kemper <jeremy@bitsweat.net>
wrote:
> Michael Koziarski wrote:
> >>Please do.  Since this is a development aid, I''d like to
be able to
> >>easily add and remove it.  Something like require
''whiny_nil'' in
> >>development (and perhaps test) environments.  Maybe
''smartass_nil'' ;)
> >
> > I opted for whiny_nil.  The patch is at
> >    http://dev.rubyonrails.org/ticket/1209
> 
> I posted a comment.
Earlier today we discussed the option of using reflection to determine
the instance_methods to handle.  However I feel it may be too
intrusive to mess with NilClass so completely.  All I''m trying to
achieve is that the most common nil mistakes are accounted for.  Does
anyone else have any thoughts on the matter?

As far as tests go,  sure.  I''ll attach them when we agree an approach
;)
> > They will now, allegedly
> 
> They do, certainly!  Thank you.
> 
> jeremy
> 

-- 
Cheers

Koz

Michael Koziarski wrote:
> On 4/28/05, Jeremy Kemper <jeremy@bitsweat.net> wrote:
> 
>>Michael Koziarski wrote:
>>
>>>>Please do.  Since this is a development aid, I''d like
to be able to
>>>>easily add and remove it.  Something like require
''whiny_nil'' in
>>>>development (and perhaps test) environments.  Maybe
''smartass_nil'' ;)
>>>
>>>I opted for whiny_nil.  The patch is at
>>>   http://dev.rubyonrails.org/ticket/1209
>>
>>I posted a comment.
> 
> 
> Earlier today we discussed the option of using reflection to determine
> the instance_methods to handle.  However I feel it may be too
> intrusive to mess with NilClass so completely.  All I''m trying to
> achieve is that the most common nil mistakes are accounted for.  Does
> anyone else have any thoughts on the matter?
> 
> As far as tests go,  sure.  I''ll attach them when we agree an
approach ;)
Same approach; more methods.  It''s already hit method_missing, after
all; there is no ''more'' or ''less'' complete. 
Why not be consistent with
the warning?

jeremy

Michael Koziarski wrote:
> On 4/28/05, Jeremy Kemper <jeremy@bitsweat.net> wrote:
> 
>>Michael Koziarski wrote:
>>
>>>On 4/28/05, Jeremy Kemper <jeremy@bitsweat.net> wrote:
>>>
>>>
>>>>Michael Koziarski wrote:
>>>>
>>>>
>>>>>>Please do.  Since this is a development aid,
I''d like to be able to
>>>>>>easily add and remove it.  Something like require
''whiny_nil'' in
>>>>>>development (and perhaps test) environments.  Maybe
''smartass_nil'' ;)
>>>>>
>>>>>I opted for whiny_nil.  The patch is at
>>>>>  http://dev.rubyonrails.org/ticket/1209
>>>>
>>>>I posted a comment.
>>>
>>>
>>>Earlier today we discussed the option of using reflection to
determine
>>>the instance_methods to handle.  However I feel it may be too
>>>intrusive to mess with NilClass so completely.  All I''m
trying to
>>>achieve is that the most common nil mistakes are accounted for. 
Does
>>>anyone else have any thoughts on the matter?
>>>
>>>As far as tests go,  sure.  I''ll attach them when we agree
an approach ;)
>>
>>Same approach; more methods.  It''s already hit method_missing,
after
>>all; there is no ''more'' or ''less''
complete.  Why not be consistent with
>>the warning?
> 
> 
> It has the additional disadvantage of making the ActiveSupport unit
> tests dependant on ActiveRecord?
This is what mocks are for.

module ActiveRecord
  class Base
    def foo; end
  end
end

Then test expecting foo only.

> I like the idea of the longer
> warning message,  I''m just not sure there''s merit in
warning about
> ActiveRecord::Base#quoted_comma_pair_list?
The merit is in consistency.  However, I don''t think there''s
much merit
in hemming and hawing over this; any of the proposals are acceptable.

Best,
jeremy

On 4/29/05, Jeremy Kemper <jeremy@bitsweat.net>
wrote:
> Michael Koziarski wrote:
> > On 4/28/05, Jeremy Kemper <jeremy@bitsweat.net> wrote:
> >
> >>Michael Koziarski wrote:
> >>
> >>>On 4/28/05, Jeremy Kemper <jeremy@bitsweat.net> wrote:
> >>>
> >>>
> >>>>Michael Koziarski wrote:
> >>>>
> >>>>
> >>>>>>Please do.  Since this is a development aid,
I''d like to be able to
> >>>>>>easily add and remove it.  Something like require
''whiny_nil'' in
> >>>>>>development (and perhaps test) environments.  Maybe
''smartass_nil'' ;)
> >>>>>
> >>>>>I opted for whiny_nil.  The patch is at
> >>>>>  http://dev.rubyonrails.org/ticket/1209
> >>>>
> >>>>I posted a comment.
> >>>
> >>>
> >>>Earlier today we discussed the option of using reflection to
determine
> >>>the instance_methods to handle.  However I feel it may be too
> >>>intrusive to mess with NilClass so completely.  All
I''m trying to
> >>>achieve is that the most common nil mistakes are accounted for.
Does
> >>>anyone else have any thoughts on the matter?
> >>>
> >>>As far as tests go,  sure.  I''ll attach them when we
agree an approach ;)
> >>
> >>Same approach; more methods.  It''s already hit
method_missing, after
> >>all; there is no ''more'' or
''less'' complete.  Why not be consistent with
> >>the warning?
> >
> >
> > It has the additional disadvantage of making the ActiveSupport unit
> > tests dependant on ActiveRecord?
> 
> This is what mocks are for.
> 
> module ActiveRecord
>   class Base
>     def foo; end
>   end
> end
> 
> Then test expecting foo only.
> 
> 
> > I like the idea of the longer
> > warning message,  I''m just not sure there''s merit in
warning about
> > ActiveRecord::Base#quoted_comma_pair_list?
> 
> The merit is in consistency.  However, I don''t think
there''s much merit
> in hemming and hawing over this; any of the proposals are acceptable.
You''ve convinced me, I''ll update the patch this weekend.
> Best,
> jeremy
> 

-- 
Cheers

Koz