commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Simone Tripodi <simonetrip...@apache.org>
Subject Re: [SANDBOX][BeanUtils2] Improve implemenation of equals() on AccessibleObjectDescriptor
Date Thu, 01 Mar 2012 19:52:59 GMT
Hi Bene,

apologize but maybe I expressed myself in the wrong form - I didn't
intend to offend you nor attack at all.
Sorry you got it personally.

My intention was rather spur you on not accepting rules/guides as they
are. I didn't hide you that IMHO you're a very talented guy - at your
age I wasn't able to contribute at your level - but it would be a
shame if you continue using someone's else techniques rather than
making your own.

> 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.

+1 that would be a very nice addition, glad if you could contribute it.

best and alles gute,
-Simo

http://people.apache.org/~simonetripodi/
http://simonetripodi.livejournal.com/
http://twitter.com/simonetripodi
http://www.99soft.org/



On Thu, Mar 1, 2012 at 6:04 PM, Benedikt Ritter
<bene@systemoutprintln.de> wrote:
> 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
>

>
> 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
>

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


Mime
View raw message