Prev: Clearing TOPLEVEL_BINDING for reuse
Next: 1.9 warning for 'private' and possible bug
From: Robert Klemme on 7 Jun 2010 13:30
On 07.06.2010 18:55, Rein Henrichs wrote:
> On 2010-06-07 02:24:26 -0700, Robert Klemme said:
>
>> 2010/6/5 Brian Candler <b.candler(a)pobox.com>:
>>> Ben Vishny wrote:
>>>> Ideally I'd like to do:
>>>>
>>>> if parent.respond_to? "children" && parent.children.size != 0
>>>> puts "Has Children"
>>>> end
>>>>
>>>> But I get an error if parent.children is undefined. Isn't there a
>>>> way of
>>>> stopping if the first condition is false?
>>>
>>> Yes, but you're being bitten by the ambiguity of what you've written,
>>> which is resolved by precedence rules. I'm not sure exactly how your
>>> code was being parsed, but I guess something like
>>>
>>> if parent.respond_to?("children" && (parent.children.size != 0))
>>>
>>> The rule is, if in doubt, add your own parentheses.
>>>
>>> The following all work as you expect:
>>>
>>> if (parent.respond_to? "children") && (parent.children.size != 0)
>>> puts "Has Children"
>>> end
>>>
>>> if parent.respond_to?("children") && parent.children.size != 0
>>> puts "Has Children"
>>> end
>>>
>>> if parent.respond_to? "children" and parent.children.size != 0
>>> puts "Has Children"
>>> end
>>>
>>> The last of these works because 'and' has very low precedence, but I'd
>>> say it's much better to be explicit with parentheses than clever with
>>> your knowledge of precedence rules.
>>>
>>> Incidentally, it's more idiomatic to use a symbol rather than a string
>>> for respond_to?
>>>
>>> if parent.respond_to?(:children) ...
>>
>> It's even more idiomatic to invoke a method and deal with the
>> exception. Checking with #respond_to? does not guarantee that the
>> method can actually be executed.
>>
>> In this case you could do
>>
>> puts "Has Children" unless parent.children.empty? rescue nil
>>
>> Or for a more targeted catch:
>>
>> begin
>> puts "Has Children" unless parent.children.empty?
>> rescue NoMethodError
>> # OK, no output
>> end

> Robert, you are correct in that "It's even more idiomatic to invoke a
> method and deal with the exception" but rescue nil is a code smell that
> I would rather see avoided. A begin/rescue block also seems unnecessary.
>
> Instead of solving a potential non-problem, I think it would be better
> to ask questions like... Why would this parent object not have a
> children method? Does this imply a design inconsistency? If parent can
> be nil, see the following.
>
> In order to avoid NoMethodError on nil bugs, it's most common to use &&,
> which short-circuits, as such:
>
> parent.children && parent.children.size != 0
>
> I may be assuming too much about your domain, but this seems more concise:
>
> parent.children && !parent.children.empty?
>
> Furthermore, it may be useful to ask: Why would children ever be nil?
> The empty array is a semantically correct way to say "no children". If
> children were initialized to [], you could always ask children.empty?
> and skip the check for nil. For instance:
>
> class Parent
> attr_reader :children
> def initialize(children=[])
> @children = children
> end
> end

Absolutely agree. At least the query for respond_to? should be avoided
since if the object does not have this method something more serious is
wrong and an exception would be the proper result.

Often nil is used as initial value for efficiency reasons or reasons for
simplicity (nil is the default for all values). So it may be
appropriate to use it for empty collection of children as well. Other
schemes could be devised as well, e.g. lazy initialization like

def children
@children ||= []
end

My main point when writing the earlier posting was that it is generally
a bad idiom to check whether something can be done before doing it. The
check gives only a false impression of safety since the operation can
fail even if the check succeeds. So it's generally better to just do it
and deal with exceptions because even with a check you have to deal with
the exception anyway. The check is basically just overhead.

Cheers

robert


--
remember.guy do |as, often| as.you_can - without end
http://blog.rubybestpractices.com/
First  |  Prev  | 
Pages: 1 2
Prev: Clearing TOPLEVEL_BINDING for reuse
Next: 1.9 warning for 'private' and possible bug