commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benedikt Ritter <b...@systemoutprintln.de>
Subject Re: [SANDBOX][BeanUtils2] Improve implemenation of equals() on AccessibleObjectDescriptor
Date Thu, 01 Mar 2012 17:04:02 GMT
Hi Simo,

I don't know, why are reacting that harshly. I think that questioning 
why an internal class does not have to obey to the general contract of 
equals() is not a sign of lacking "spirit of criticism".
I think adding that check or suppressing a FindBugs complain are both 
equally valid (although the first one IMHO is less obscure). Even though 
you're right, when saying that ATM that can never happen.

Regards,
Benedikt

PS: if you are going for performance you could store the hash code in a 
private filed after the first computation and return the computed value 
on subsequent invocations.

Am 01.03.2012 17:11, schrieb Simone Tripodi:
>> 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
> <bene@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
>>> <bene@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-unsubscribe@commons.apache.org
>>>> For additional commands, e-mail: dev-help@commons.apache.org
>>>>
>>>
>>> ---------------------------------------------------------------------
>>> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
>>> For additional commands, e-mail: dev-help@commons.apache.org
>>
>>
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
>> For additional commands, e-mail: dev-help@commons.apache.org
>>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org


Mime
View raw message