> if ( !( obj instanceof AccessibleObjectDescriptor ) )
> {
> return false;
> }
what is the sense? having a situation where AccessibleObjectDescriptor
is compared to a different type object is something that can simply
*never* happen!
AccessibleObjectDescriptor is a *private static* class of the
AccessibleObjectRegistry - so even the other BeanUtils2 classes know
that it is living under the same umbrella - which visibility scope is
limited to the beanutils2 package.
> That will make AccessibleObjectDescritpor.equals() obey to the general
> contract of equals (which states, that x.equals(null) has to return false)
again, explain why it should be useful under the known circumstances.
> and it will fix the FindBugs issue, which will have to be fixed anyway,
FindBugs violations can be suppressed, and fortunately this is one of
the rare occasions we can do it.
> if BeanUtils2 leaves Sandbox and gets released someday (at least I hope that
> FindBugs understands, that null instanceof WhatEver returns false).
If you want to apply all the best practice you should check every aspect:
+---+
if ( this == obj )
{
return true;
}
if ( obj == null )
{
return false;
}
if ( getClass() != obj.getClass() ) // or manage in
whatever is your preferred way
{
return false;
}
+---+
the first check is missing and it is something that would increase the
performance, so I intend to commit it.
> I see no reason to write less robust code, just because it is internal to
> the library and saves us a few lines.
And I see no reason why you intend applying rules without using a
minimum spirit of criticism. If you analyze the context in which that
class participate, instead of reading the code and se what should/what
not shall has to be done, you can see that cases you intend to cover
*can never happen*.
And just to make it clear: I am not a moron which matter is just of
saving lines of code, it is a metter of using stuff when they are
required - and NOT using them if they are not needed.
http://people.apache.org/~simonetripodi/
http://simonetripodi.livejournal.com/
http://twitter.com/simonetripodi
http://www.99soft.org/
On Thu, Mar 1, 2012 at 4:07 PM, Benedikt Ritter
<b...@systemoutprintln.de> wrote:
> The only thing we have to add is
>
> if ( !( obj instanceof AccessibleObjectDescriptor ) )
> {
> return false;
> }
>
> That will make AccessibleObjectDescritpor.equals() obey to the general
> contract of equals (which states, that x.equals(null) has to return false)
> and it will fix the FindBugs issue, which will have to be fixed anyway, if
> BeanUtils2 leaves Sandbox and gets released someday (at least I hope that
> FindBugs understands, that null instanceof WhatEver returns false).
> I see no reason to write less robust code, just because it is internal to
> the library and saves us a few lines.
>
> Am 01.03.2012 15:49, schrieb Simone Tripodi:
>
>> AccessibleObjectRegistry.AccessibleObjectDescriptor is used internally
>> only, users don't even know that it exist and it is used only as a key
>> for the AccessibleObject index.
>> Does it make sense checking other types, nulls, assignability from
>> super/subclasses, ... etc?
>>
>> http://people.apache.org/~simonetripodi/
>> http://simonetripodi.livejournal.com/
>> http://twitter.com/simonetripodi
>> http://www.99soft.org/
>>
>>
>>
>> On Thu, Mar 1, 2012 at 3:09 PM, Benedikt Ritter
>> <b...@systemoutprintln.de> wrote:
>>>
>>> Hi,
>>>
>>> I just ran the eclipse FindBugs plugin with default configuration on
>>> BeanUtils2 and it pointed me to equals() in
>>> AccessibleObjectRegistry.AccessibleObjectDescriptor, reporting that the
>>> cast
>>> in line 535
>>>
>>> AccessibleObjectDescriptor other = (AccessibleObjectDescriptor) obj;
>>>
>>> is not checked for null.
>>> Now I'd like to implement equals() like it is shown in Effective Java.
>>> Are
>>> there any arguments against changing the implementation that way?
>>>
>>> Regards,
>>> Benedikt
>>>
>>> ---------------------------------------------------------------------
>>> To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
>>> For additional commands, e-mail: dev-h...@commons.apache.org
>>>
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
>> For additional commands, e-mail: dev-h...@commons.apache.org
>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
> For additional commands, e-mail: dev-h...@commons.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org
