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