[prev in list] [next in list] [prev in thread] [next in thread] 

List:       jakarta-commons-dev
Subject:    [jira] Commented: (LANG-467) EqualsBuilder and HashCodeBuilder
From:       "David Jones (JIRA)" <jira () apache ! org>
Date:       2008-10-27 16:46:44
Message-ID: 1703193612.1225126004303.JavaMail.jira () brutus
[Download RAW message or body]


    [ https://issues.apache.org/jira/browse/LANG-467?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12642958#action_12642958 \
] 

David Jones commented on LANG-467:
----------------------------------

The use of compareTo() in EqualsBuilder() is arguably wrong, however compareTo is \
referenced from the BigDecimal javadoc as the alternative to equals for cases where \
the scale is not relevant

In LANG-393 it was decided that EqualsBuidler should use the compareTo() method to \
compare BigDecimals(), which is a nice convenience for those of use using BigDecimals \
in conjunction with EqualsBuilder and who want 0 and 0.0 to be considered equal.

However LANG-393 did not put the equivalent fix into the HashCodeBuilder so this has \
made HashCodeBuilder and EqualsBuilder non-symmetric.

So as far as the EqualsBuilder is concerned 10.2 and 10.20 are equal, i.e. the \
following evaluates to true.  {code}
new EqualsBuilder().append(new BigDecimal("10.2"), new \
BigDecimal("10.20")).isEquals(); {code}

However when using these two values with HashCodeBuilder they actually give different \
hashCodes(), the following evaluates to false. {code}
new HashCodeBuilder(17, 37).append(new BigDecimal("10.2")) == new HashCodeBuilder(17, \
37).append(new BigDecimal("10.20")) {code}

However the contract of hashCode() method for Object says that if two objects are \
considered equal using their equals method then they must also generate the same \
hashCode().

Of course this is true for BigDecimal class itself, even though it is somewhat \
inconvenient. 

MyPojo class as given in the test case above 
* implements the equals() method as documented by EqualsBuilder
* implements the hashCode() method as documented by HashCodeBuilder

Despite following the documented approach for implementing equals and hashCode the \
test case *proves* that MyPojo breaks the contract of hashCode(), the following \
evaluates to true {code}
myPojo1.equals(myPojo2)
{code}
However myPojo1 and myPojo2 generate different hashCodes(), the following evaluates \
to false {code}
myPojo1.hashCode() == myPojo2.hashCode()
{code}


> EqualsBuilder and HashCodeBuilder treat java.math.BigDecimal inconsistantly and \
>                 break general contract of hashCode
> ------------------------------------------------------------------------------------------------------------------
>  
> Key: LANG-467
> URL: https://issues.apache.org/jira/browse/LANG-467
> Project: Commons Lang
> Issue Type: Bug
> Affects Versions: 2.4
> Reporter: David Jones
> Priority: Minor
> 
> A POJO with a BigDecimal field and equals() and hashCode() methods implemented \
> using EqualsBuilder and HashCodeBuilder breaks the general contract of \
> Object.hashCode(); EqualsBuilder treats BigDecimal specially by comparing it using \
> BigDecimal.compareTo() == 0 rather than BigDecimal.equals() Using \
> BigDecimal.compareTo() ignores the scale of the BigDecimal() However the \
> append(Object o) method of HashCodeBuilder uses BigDecimal.hashCode() in the case \
> that o is a BigDecimal, which takes the scale into account when generating the \
> hashCode. The following test case shows the problem!
> {code:title=TestApacheCommonsLangHashCodeBuilder.java|borderStyle=solid}
> // package declaration and imports not shown
> public class TestApacheCommonsLangHashCodeBuilder extends TestCase {
> 
> public void testHashCode() {
> MyPojo myPojo1 = new MyPojo(new String("foo"), new BigDecimal("10.2"));
> MyPojo myPojo2 = new MyPojo(new String("foo"), new BigDecimal("10.20"));
> 
> // equals method ignores the scale of the big decimal
> // so this test passes
> assertTrue(myPojo1.equals(myPojo2));
> 
> // however in the case the equals returns true the
> // hashCode must be the same according to the contract
> // TEST FAILS AT THIS LINE
> assertEquals(myPojo1.hashCode(), myPojo2.hashCode());
> }
> 
> private class MyPojo {
> private String name;
> private BigDecimal value;
> 
> public MyPojo(String name, BigDecimal value) {
> this.name = name;
> this.value = value;
> }
> 
> public String getName() {
> return name;
> }
> public BigDecimal getValue() {
> return value;
> }
> /**
> * equals method implemented using EqualsBuilder 
> * as documented by apache commons
> */
> @Override public boolean equals(Object obj) {
> if(this == obj) {
> return true;
> }
> 
> if(!(obj instanceof MyPojo)) {
> return false;
> }
> 
> MyPojo other = (MyPojo) obj;
> return new EqualsBuilder()
> .append(name, other.getName())
> .append(value, other.getValue())
> .isEquals();
> }
> 
> /**
> * hashCode method implemented using HashCodeBuilder
> * as documented by apache commons
> */
> @Override public int hashCode() {
> HashCodeBuilder hcb = new HashCodeBuilder(17, 31);
> return hcb
> .append(name)
> .append(value)
> .toHashCode();
> }
> }
> }
> {code}
> Note that the only reason I haven't provided a patch is because I could not think \
>                 of one which I thought was reasonable.
> *Option 1*
> Always set the scale to some value and then get the hashCode()
> Example change in HashCodeBuilder.append(Object) add the following:
> {code}
> else if (object instanceof BigDecimal) {
> 	append(((BigDecimal) object).setScale(DEFAULT_BIGDECIMAL_SCALE, \
> RoundingMode.DOWN).hashCode()); }
> {code}
> However what is a reasonable scale for calculating this hashCode? I cannot see a \
>                 reasonanble scale to choose, that depends on the circumstance
> *Option 2*
> stripTrailingZeros() before calculating the hashCode()
> Example change in HashCodeBuilder.append(Object) add the following:
> {code}
> else if (object instanceof BigDecimal) {
> 	append(((BigDecimal) object).stripTrailingZeros().hashCode());
> }
> {code}
> The performance of this method under different circumstances is not documented.
> *Option 3*
> Document the problem and propose that the client solves the problem.
> For example change HashCodeBuilder documentation as follows
> {code}
> /*
> * ...
> * public class Person {
> *   String name;
> *   int age;
> *   boolean smoker;
> *   BigDecimal netWorth;
> *   ...
> *
> *   public int hashCode() {
> *     // you pick a hard-coded, randomly chosen, non-zero, odd number
> *     // ideally different for each class
> *     return new HashCodeBuilder(17, 37).
> *       append(name).
> *       append(age).
> *       append(smoker).
> *       // take special care when using BigDecimal as scale takes 
> *       // is included in the hashCode calculation breaking hashCode contract
> *       // choose a scale which is reasonable for hashCode calculation
> *       append(netWorth == null ? null : netWorth.setScale(2)).
> *       toHashCode();
> *   }
> * }
> * ...
> */
> {code}

-- 
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.


[prev in list] [next in list] [prev in thread] [next in thread]