You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hc.apache.org by Gary Gregory <ga...@gmail.com> on 2019/12/25 21:45:52 UTC
Re: [httpcomponents-core] 01/01: Normalize TimeUnit in TimeValue's
equals() and hashCode()
This is terrible IMO and as close to a -1 from me as can be. It gives the
false impression that we implement equals() and hashCode(). Not only that,
but this impression is reinforced by the lack of comments so it looks like
a bug because we did not think about the implemetation carefully enough :-(
If you find my approach in the branch too 'heavy' then let's at least
change scale() to return a BigInt and use that to internally convert to
BigInt Nanos and then do comparisons.
Thoughts?
Gary
On Wed, Dec 25, 2019 at 1:39 PM <mi...@apache.org> wrote:
> This is an automated email from the ASF dual-hosted git repository.
>
> michaelo pushed a commit to branch TimeValue-equals-hashCode
> in repository https://gitbox.apache.org/repos/asf/httpcomponents-core.git
>
> commit 2bb2c3872da044eab68f1bb800d420378bc80a95
> Author: Michael Osipov <mi...@apache.org>
> AuthorDate: Wed Dec 25 19:38:33 2019 +0100
>
> Normalize TimeUnit in TimeValue's equals() and hashCode()
> ---
> httpcore5/src/main/java/org/apache/hc/core5/util/TimeValue.java | 8
> +++++---
> .../src/test/java/org/apache/hc/core5/util/TestTimeValue.java | 6
> ++++--
> 2 files changed, 9 insertions(+), 5 deletions(-)
>
> diff --git
> a/httpcore5/src/main/java/org/apache/hc/core5/util/TimeValue.java
> b/httpcore5/src/main/java/org/apache/hc/core5/util/TimeValue.java
> index 576d875..156ff40 100644
> --- a/httpcore5/src/main/java/org/apache/hc/core5/util/TimeValue.java
> +++ b/httpcore5/src/main/java/org/apache/hc/core5/util/TimeValue.java
> @@ -229,7 +229,9 @@ public class TimeValue implements
> Comparable<TimeValue> {
> }
> if (obj instanceof TimeValue) {
> final TimeValue that = (TimeValue) obj;
> - return this.duration == that.duration &&
> LangUtils.equals(this.timeUnit, that.timeUnit);
> + final long thisDuration = this.convert(TimeUnit.NANOSECONDS);
> + final long thatDuration = that.convert(TimeUnit.NANOSECONDS);
> + return thisDuration == thatDuration;
> }
> return false;
> }
> @@ -274,8 +276,8 @@ public class TimeValue implements
> Comparable<TimeValue> {
> @Override
> public int hashCode() {
> int hash = LangUtils.HASH_SEED;
> - hash = LangUtils.hashCode(hash, duration);
> - hash = LangUtils.hashCode(hash, timeUnit);
> + hash = LangUtils.hashCode(hash,
> this.convert(TimeUnit.NANOSECONDS));
> + hash = LangUtils.hashCode(hash, TimeUnit.NANOSECONDS);
> return hash;
> }
>
> diff --git
> a/httpcore5/src/test/java/org/apache/hc/core5/util/TestTimeValue.java
> b/httpcore5/src/test/java/org/apache/hc/core5/util/TestTimeValue.java
> index 38c3b3f..f9724f3 100644
> --- a/httpcore5/src/test/java/org/apache/hc/core5/util/TestTimeValue.java
> +++ b/httpcore5/src/test/java/org/apache/hc/core5/util/TestTimeValue.java
> @@ -275,12 +275,14 @@ public class TestTimeValue {
> Assert.assertThat(tv1.equals(null), CoreMatchers.equalTo(false));
> Assert.assertThat(tv1.equals(tv2), CoreMatchers.equalTo(false));
> Assert.assertThat(tv1.equals(tv3), CoreMatchers.equalTo(true));
> - Assert.assertThat(tv1.equals(tv4), CoreMatchers.equalTo(false));
> + Assert.assertThat(tv1.equals(tv4), CoreMatchers.equalTo(true));
> + Assert.assertThat(tv4.equals(tv1), CoreMatchers.equalTo(true));
> Assert.assertThat(tv1.equals(tv5), CoreMatchers.equalTo(false));
>
> Assert.assertThat(tv1.hashCode() == tv2.hashCode(),
> CoreMatchers.equalTo(false));
> Assert.assertThat(tv1.hashCode() == tv3.hashCode(),
> CoreMatchers.equalTo(true));
> - Assert.assertThat(tv1.hashCode() == tv4.hashCode(),
> CoreMatchers.equalTo(false));
> + Assert.assertThat(tv1.hashCode() == tv4.hashCode(),
> CoreMatchers.equalTo(true));
> + Assert.assertThat(tv4.hashCode() == tv1.hashCode(),
> CoreMatchers.equalTo(true));
> Assert.assertThat(tv1.hashCode() == tv5.hashCode(),
> CoreMatchers.equalTo(false));
> }
>
>
>
Re: [httpcomponents-core] 01/01: Normalize TimeUnit in TimeValue's
equals() and hashCode()
Posted by Michael Osipov <mi...@apache.org>.
Am 2019-12-25 um 22:45 schrieb Gary Gregory:
> This is terrible IMO and as close to a -1 from me as can be. It gives the
> false impression that we implement equals() and hashCode(). Not only that,
> but this impression is reinforced by the lack of comments so it looks like
> a bug because we did not think about the implemetation carefully enough :-(
> If you find my approach in the branch too 'heavy' then let's at least
> change scale() to return a BigInt and use that to internally convert to
> BigInt Nanos and then do comparisons.
>
> Thoughts?
Please beware that this is a naive implemenation. I am OK using
BigInteger internally, but if even so. Don't you think that long is long
enough even if we scale everything to nanoseconds?
Morever, as you have noticed I have scaled everything to nanoseconds in
hashCode() otherwise it won't work out.
I'd be happy to review an updated PR of yours!
Michael
> On Wed, Dec 25, 2019 at 1:39 PM <mi...@apache.org> wrote:
>
>> This is an automated email from the ASF dual-hosted git repository.
>>
>> michaelo pushed a commit to branch TimeValue-equals-hashCode
>> in repository https://gitbox.apache.org/repos/asf/httpcomponents-core.git
>>
>> commit 2bb2c3872da044eab68f1bb800d420378bc80a95
>> Author: Michael Osipov <mi...@apache.org>
>> AuthorDate: Wed Dec 25 19:38:33 2019 +0100
>>
>> Normalize TimeUnit in TimeValue's equals() and hashCode()
>> ---
>> httpcore5/src/main/java/org/apache/hc/core5/util/TimeValue.java | 8
>> +++++---
>> .../src/test/java/org/apache/hc/core5/util/TestTimeValue.java | 6
>> ++++--
>> 2 files changed, 9 insertions(+), 5 deletions(-)
>>
>> diff --git
>> a/httpcore5/src/main/java/org/apache/hc/core5/util/TimeValue.java
>> b/httpcore5/src/main/java/org/apache/hc/core5/util/TimeValue.java
>> index 576d875..156ff40 100644
>> --- a/httpcore5/src/main/java/org/apache/hc/core5/util/TimeValue.java
>> +++ b/httpcore5/src/main/java/org/apache/hc/core5/util/TimeValue.java
>> @@ -229,7 +229,9 @@ public class TimeValue implements
>> Comparable<TimeValue> {
>> }
>> if (obj instanceof TimeValue) {
>> final TimeValue that = (TimeValue) obj;
>> - return this.duration == that.duration &&
>> LangUtils.equals(this.timeUnit, that.timeUnit);
>> + final long thisDuration = this.convert(TimeUnit.NANOSECONDS);
>> + final long thatDuration = that.convert(TimeUnit.NANOSECONDS);
>> + return thisDuration == thatDuration;
>> }
>> return false;
>> }
>> @@ -274,8 +276,8 @@ public class TimeValue implements
>> Comparable<TimeValue> {
>> @Override
>> public int hashCode() {
>> int hash = LangUtils.HASH_SEED;
>> - hash = LangUtils.hashCode(hash, duration);
>> - hash = LangUtils.hashCode(hash, timeUnit);
>> + hash = LangUtils.hashCode(hash,
>> this.convert(TimeUnit.NANOSECONDS));
>> + hash = LangUtils.hashCode(hash, TimeUnit.NANOSECONDS);
>> return hash;
>> }
>>
>> diff --git
>> a/httpcore5/src/test/java/org/apache/hc/core5/util/TestTimeValue.java
>> b/httpcore5/src/test/java/org/apache/hc/core5/util/TestTimeValue.java
>> index 38c3b3f..f9724f3 100644
>> --- a/httpcore5/src/test/java/org/apache/hc/core5/util/TestTimeValue.java
>> +++ b/httpcore5/src/test/java/org/apache/hc/core5/util/TestTimeValue.java
>> @@ -275,12 +275,14 @@ public class TestTimeValue {
>> Assert.assertThat(tv1.equals(null), CoreMatchers.equalTo(false));
>> Assert.assertThat(tv1.equals(tv2), CoreMatchers.equalTo(false));
>> Assert.assertThat(tv1.equals(tv3), CoreMatchers.equalTo(true));
>> - Assert.assertThat(tv1.equals(tv4), CoreMatchers.equalTo(false));
>> + Assert.assertThat(tv1.equals(tv4), CoreMatchers.equalTo(true));
>> + Assert.assertThat(tv4.equals(tv1), CoreMatchers.equalTo(true));
>> Assert.assertThat(tv1.equals(tv5), CoreMatchers.equalTo(false));
>>
>> Assert.assertThat(tv1.hashCode() == tv2.hashCode(),
>> CoreMatchers.equalTo(false));
>> Assert.assertThat(tv1.hashCode() == tv3.hashCode(),
>> CoreMatchers.equalTo(true));
>> - Assert.assertThat(tv1.hashCode() == tv4.hashCode(),
>> CoreMatchers.equalTo(false));
>> + Assert.assertThat(tv1.hashCode() == tv4.hashCode(),
>> CoreMatchers.equalTo(true));
>> + Assert.assertThat(tv4.hashCode() == tv1.hashCode(),
>> CoreMatchers.equalTo(true));
>> Assert.assertThat(tv1.hashCode() == tv5.hashCode(),
>> CoreMatchers.equalTo(false));
>> }
>>
>>
>>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org