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 21:27:02 GMT
Am 01.03.2012 20:52, schrieb Simone Tripodi:
> 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.

you're right, I got that wrong - sorry (it's a bit ironic, since in my 
last mail I told you not to worry about stuff like that ;). Let's go 
back to business!

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

I'll right a patch tomorrow.
Buona notte! ;)
Benedikt

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


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


Mime
View raw message