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
<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
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org
