commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From scolebou...@btopenworld.com
Subject Re: [lang] EqualsBuilder
Date Thu, 12 Sep 2002 09:20:11 GMT

>  from:    Steve Downey <steve.downey@netfolio.com>
> > 1) I think that the reflection method should be static, and take the
> > transient flag as a boolean parameter. The two techniques are distinct.
> > this way forces it.
> > return EqualsBuilder.reflectionEquals(obj1, obj2);
> > return EqualsBuilder.reflectionEquals(obj1, obj2, boolean
> > includeTransient);
> >
> I thought that too, at first, but the reflectionEquals (good name) method 
> needs to call append(), and append() is NOT side effect free. It sets the 
> isEquals flag on the EqualsBuilder object.  
> 
> Also, since the normal case requires an object, it's possible to call 
> reflectionEquals on an instance. If by some means reflectionEquals didn't use 
> the isEquals property, it could lead to subtle errors, since isEquals would 
> always be true.

The solution is to create a new EqualsBuilder instance within the static reflectionEquals
method. This fully keeps the separation, and avoids those subtle errors. 

Stephen


------------------------------
> > 2) The Object[] instanceof is coded in the Object[] method. I found that it
> > needs to be coded in the Object method. For example:
> > Object obj = new long[2];
> > is allowed, thus could be passed into the append(Object) method.
> 
> Perverse, but possible. Why in hell aren't the <primitive>[] classes derived 
> from something other than Object. Something that overrode equals, hashCode, 
> and so forth.
> 
> I even ran into this problem in building the test cases. 
> 
> I looked into dealing with aliased arrays in reflectionEquals. It got very 
> ugly, very fast, so I didn't.  It think it's outside the use case, anyway. 
> One of the inputs to reflectionEquals should be 'this', and 'this' can't be 
> an array.
> 
> Changes made, tests added, and new files attached.
> 
> And I retract my earliest comment about this being a conceptually lightweight 
> class. There are a lot of ugly corner cases that make writing an equals 
> method hard.
> 
> >
> > 3) The long[] etc. methods don't seem to check for nulls.
> >
> 
> > Stephen
> >
> > ----- Original Message -----
> > From: "Steve Downey" <steve.downey@netfolio.com>
> > To: "Jakarta Commons Developers List" <commons-dev@jakarta.apache.org>
> > Sent: Wednesday, September 11, 2002 1:24 AM
> > Subject: Re: [lang] EqualsBuilder
> >
> >
> > Here's the updated version of EqualsBuilder. The multi-dim array stuff was
> > particularly interesting.
> >
> > On Monday 09 September 2002 08:58 pm, Steve Downey wrote:
> > > On Monday 09 September 2002 06:49 pm, Stephen Colebourne wrote:
> > > > Comments:
> > > >
> > > > 1) I would use append as the method name. HashCode and ToString both
> > > > use append - it seems to be a mini pattern.
> > >
> > > No problem. That's a simple search and replace.
> > >
> > > > 2) The double and float use an unusual comparison strategy. Was there
a
> > > > reason for this?
> > >
> > > Yes. Otherwise you can get bolixed up with NaN's, -0s, INFs and such. See
> > > for example Effective Java Item 7, point 4.  I'll add javadoc to explain
> > > what's going on.
> > >
> > > > 3) Double and float could also have an allowed delta version. This
> > > > would operate similarly to JUnit asserts.
> > >
> > > I disagree. In the context of equals(), you will violate the contract if
> > > you use a tolerance. Two items that are equal will have different hash
> > > codes. This is also part of the reason for point 2.
> > >
> > > It's critical that two objects that are equal behave identically. No
> >
> > matter
> >
> > > how small the tolerances, there are plenty of situations where the
> >
> > behavior
> >
> > > will diverge. Repeated multiplications by two is a trivial example.
> > >
> > > I'm looking at CompareToBuilder, also. The same problem applies there. If
> > > you have a tolerance you end up with compareTo(a,b) == 0, compareTo(b,c)
> >
> > ==
> >
> > > 0, and compareTo(a,c) != 0.
> > >
> > > > 4) The array handling has to be more complex to handle multi
> > > > dimensional arrays. See ToStringBuilder. (Note to myself, I need to do
> > > > this to HashCodeBuilder before a release...)
> > >
> > > Umm. I didn't run through it in depth, but I'm not seeing any handling of
> > > multi-dimensional arrays in the ToStringBuilder that's in
> > > commons-sandbox. I'm looking at version 1.2. It imports Array, Map and
> > > Collection, but doesn't reference them.
> > > OK, now I see it. In ToStringStyle, Object[] checks if the elements are
> > > array types themselves. That's doable. And starts to add to the real
> > > utility of the class, since it's tedious and errorprone to do correctly
> > > by hand.
> > >
> > > > The reflection version definitely helps add usefulness as well. Similar
> > > > code should be added to HashCodeBuilder and ToStringBuilder. It might
> > > > be better with a 'don't include transient fields' flag.
> > >
> > > That's straightforward and useful. Transient fields are, almost by
> > > definition, not part of the state of an object. [OK, sometimes they are,
> > > but it's usually a bug.] The default should probably be to exclude them.
> > > A flag could include them
> > >
> > > > So my vote is  1 with a little more work (as with all the builders at
> > > > present)
> > > >
> > > > Stephen
> > >
> > > I'll make the changes and repost.
> > >
> > > > ----- Original Message -----
> > > > From: "Steve Downey" <steve.downey@netfolio.com>
> > > > I saw this in the Todo's, and put it together. After building it, I'm
> >
> > not
> >
> > > > really sure it pulls it's own weight. Use is along these lines
> > > >
> > > > public boolean equals(Object o) {
> > > >   if (!o instanceof MyClass) {
> > > >     return false;
> > > >   }
> > > >   MyClass rhs = (MyClass) o;
> > > >   return new EqualsBuilder()
> > > >                   .test(field1, rhs.field1)
> > > >                   .test(field2, rhs.field2)
> > > >                   .test(field3, rhs.field3)
> > > >   .isEquals();
> > > >
> > > > }
> > > >
> > > > or
> > > >
> > > > public boolean equals(Object o) {
> > > >   return = new EqualsBuilder().reflectionTest(this,o);
> > > > }
> > > >
> > > > the second form uses the setAccessible(true) hack, and so will throw
> > > > exceptions under a security manager. OTOH, it's simple for testing
> > > > code, which is where I seem to use equals most of the time. E.g. if a
> > > > round trip through the database leaves an object equal to itself.
> > > >
> > > > IAC, I haven't done all the JavaDoc comments. I will if anyone thinks
> > > > this would be useful for lang.
> > >
> > > -------------------------------------------------------------------------
> > >
> > > >-- - ----
> > > >
> > > > > --
> > > > > To unsubscribe, e-mail:
> > > >
> > > > <mailto:commons-dev-unsubscribe@jakarta.apache.org>
> > > >
> > > > > For additional commands, e-mail:
> > > >
> > > > <mailto:commons-dev-help@jakarta.apache.org>
> >
> > ---------------------------------------------------------------------------
> >- ----
> >
> > > --
> > > To unsubscribe, e-mail:
> >
> > <mailto:commons-dev-unsubscribe@jakarta.apache.org>
> >
> > > For additional commands, e-mail:
> >
> > <mailto:commons-dev-help@jakarta.apache.org>


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


Mime
View raw message