Add hashCode() to classes with equals() and clean up equals() #1222

jmthibault79 · 2018-11-07T19:15:02Z

Description

equals() without hashCode() is bad news. Also, some equals() have problems too.

Checklist

Code compiles correctly
New tests covering changes and new functionality
All tests passing
Extended the README / documentation, if necessary
Is not backward compatible (breaks binary or source compatibility)

jmthibault79 · 2018-11-07T19:33:34Z

~~Also, fix 3 classes' equals()~~ Scope expanded to cover more equals()

jmthibault79 · 2018-11-07T19:34:05Z

src/main/java/htsjdk/samtools/cram/encoding/readfeatures/Bases.java

@@ -65,8 +66,12 @@ public boolean equals(final Object obj) {

        final Bases bases = (Bases) obj;

-        return position == bases.position && !Arrays.equals(this.bases, bases.bases);
+        return position == bases.position && Arrays.equals(this.bases, bases.bases);


jmthibault79 · 2018-11-07T19:34:23Z

src/main/java/htsjdk/samtools/cram/encoding/readfeatures/Deletion.java

@@ -64,10 +65,14 @@ public boolean equals(final Object obj) {
        if (!(obj instanceof Deletion))
            return false;

-        final Deletion deleteion = (Deletion) obj;
+        final Deletion other = (Deletion) obj;


codecov-io · 2018-11-07T19:34:53Z

Codecov Report

Merging #1222 into master will decrease coverage by 0.04%.
The diff coverage is 6.78%.

@@              Coverage Diff               @@
##              master     #1222      +/-   ##
==============================================
- Coverage     69.016%   68.975%   -0.04%     
- Complexity      8066      8072       +6     
==============================================
  Files            539       539              
  Lines          32552     32587      +35     
  Branches        5488      5510      +22     
==============================================
+ Hits           22466     22477      +11     
- Misses          7890      7903      +13     
- Partials        2196      2207      +11

Impacted Files	Coverage Δ	Complexity Δ
src/main/java/htsjdk/tribble/index/Block.java	`56.25% <0%> (-3.75%)`	`6 <1> (ø)`
...k/samtools/cram/encoding/readfeatures/Padding.java	`36.842% <0%> (-2.047%)`	`4 <0> (ø)`
...k/samtools/cram/encoding/readfeatures/RefSkip.java	`36.842% <0%> (-2.047%)`	`4 <0> (ø)`
.../samtools/cram/encoding/readfeatures/Deletion.java	`36.842% <0%> (-2.047%)`	`4 <0> (ø)`
.../samtools/cram/encoding/readfeatures/ReadBase.java	`36% <0%> (-1.5%)`	`5 <0> (ø)`
.../java/htsjdk/tribble/index/linear/LinearIndex.java	`77.907% <0%> (-0.456%)`	`18 <0> (ø)`
.../samtools/cram/encoding/readfeatures/HardClip.java	`36.842% <0%> (-2.047%)`	`4 <0> (ø)`
...samtools/cram/encoding/readfeatures/Insertion.java	`31.818% <0%> (-7.071%)`	`4 <0> (ø)`
...amtools/cram/encoding/readfeatures/InsertBase.java	`65% <0%> (-3.421%)`	`7 <0> (ø)`
...huffman/codec/CanonicalHuffmanIntegerEncoding.java	`76.923% <0%> (-8.791%)`	`10 <0> (ø)`
... and 12 more

jmthibault79 · 2018-11-07T19:35:42Z

src/main/java/htsjdk/samtools/cram/structure/CramCompressionRecord.java

@@ -125,6 +126,15 @@ public boolean equals(final Object obj) {

    }

+    @Override
+    public int hashCode() {
+        // Note: flags covers these checks as well:


I'll probably revisit this when I refactor CramCompressionRecord in earnest

lbergelson

@jmthibault79 Thanks for doing this. I think there are still a bunch of problems here due to the annoying fact that Array doesn't override Object.hashCode.

lbergelson · 2018-11-07T20:16:48Z

src/main/java/htsjdk/samtools/cram/encoding/readfeatures/Bases.java


+    @Override
+    public int hashCode() {
+        return Objects.hash(position, bases);


Won't this use the memory location of bases instead of the base values?

lbergelson · 2018-11-07T20:19:08Z

src/main/java/htsjdk/samtools/cram/encoding/huffman/codec/CanonicalHuffmanIntegerEncoding.java

+
+    @Override
+    public int hashCode() {
+        return Objects.hash(bitLengths, values);


I think this just uses the default hashcode, not equivalent to Arrays.hashCode().

lbergelson · 2018-11-07T20:20:46Z

src/main/java/htsjdk/samtools/cram/common/Version.java

@@ -52,11 +54,16 @@ public int compareTo(@SuppressWarnings("NullableProblems") final Version o) {
     */
    @Override
    public boolean equals(final Object obj) {
-        if (obj == null || !(obj instanceof Version)) return false;
+        if (obj == null || (obj.getClass() != getClass())) return false;


It seems like this isn't consistent with the compareTo method since that takes build into account as well. You don't have to make it so but you should add a comment if you don't.

lbergelson · 2018-11-07T20:22:55Z

src/main/java/htsjdk/samtools/cram/encoding/readfeatures/Insertion.java


        final Insertion insertion = (Insertion) obj;

        return position == insertion.position && Arrays.equals(sequence, insertion.sequence);
    }

+    @Override
+    public int hashCode() {
+        return Objects.hash(position, sequence);


same problem with Object.equals vs Arrays.equals.

lbergelson · 2018-11-07T20:24:13Z

src/main/java/htsjdk/samtools/cram/encoding/readfeatures/Scores.java


+    @Override
+    public int hashCode() {
+        return Objects.hash(position, scores);


lbergelson · 2018-11-07T20:30:48Z

src/main/java/htsjdk/samtools/cram/structure/CramCompressionRecord.java

+        // Note: flags covers these checks as well:
+        // isNegativeStrand(), isVendorFiltered(), isSegmentUnmapped(), isLastSegment(), isFirstSegment()
+
+        return Objects.hash(alignmentStart, flags, readLength, recordsToNextFragment, mappingQuality, readFeatures,


same problem with arrays

lbergelson · 2018-11-07T20:40:52Z

src/main/java/htsjdk/samtools/cram/structure/CramHeader.java

@@ -103,6 +103,10 @@ public boolean equals(final Object obj) {
        return Arrays.equals(id, header.id) && getSamFileHeader().equals(header.getSamFileHeader());
    }

+    @Override
+    public int hashCode() {
+        return Objects.hash(version, id, samFileHeader);


Be aware that if you update version.hashCode to take build into account that now this will no longer be correct since this doesn't compare with version.equals.

lbergelson · 2018-11-07T20:47:12Z

src/main/java/htsjdk/samtools/cram/structure/ReadTag.java

@@ -154,17 +155,15 @@ public int compareTo(@SuppressWarnings("NullableProblems") final ReadTag o) {

    @Override
    public boolean equals(final Object obj) {
-        if (!(obj instanceof ReadTag))
-            return false;
+        if (obj == null || (obj.getClass() != getClass())) return false;

        final ReadTag foe = (ReadTag) obj;
        return key.equals(foe.key) && (value == null && foe.value == null || value != null && value.equals(foe.value));


This definition and it's hashCode are correctly matching now. There's a problem with the equals method for cases of 'H' and 'B'. 'H' returns an array which should be compared using Arrays.equals() , and 'B' returns a TagValueAndUnsignedArrayFlag which doesn't implement equals.

Fixing the array case will require a corresponding change to the hashCode.

lbergelson · 2018-11-07T20:56:43Z

src/main/java/htsjdk/samtools/reference/FastaSequenceIndex.java

@@ -135,6 +132,11 @@ public boolean equals(Object other) {
        return true;
    }

+    @Override


I think this really has to hash every element of sequenceEntries.

lbergelson · 2018-11-07T20:59:19Z

src/main/java/htsjdk/samtools/cram/encoding/readfeatures/Substitution.java

@@ -116,6 +116,11 @@ public boolean equals(final Object obj) {
        return true;
    }

+    @Override
+    public int hashCode() {
+        return Objects.hash(position, code, referenceBase, base);


I think this has to be clever about the case where code = NO_CODE and ignore referenceBase and base in that case.

jmthibault79 · 2018-11-09T14:18:35Z

src/main/java/htsjdk/samtools/cram/encoding/huffman/codec/CanonicalHuffmanIntegerEncoding.java


 public class CanonicalHuffmanIntegerEncoding implements Encoding<Integer> {
    private static final EncodingID ENCODING_ID = EncodingID.HUFFMAN;
    private int[] bitLengths;
    private int[] values;
-    private final ByteBuffer buf = ByteBuffer.allocate(1024 * 10);


Moved inside method where it belongs, and it shouldn't be used for equality tests

I don't think you want to localize this to the method it's used in, because that means re-allocating it every time. That seems like it could be a substantial performance hit through increased garbage collection.

You can just ignore it in equality checks.

good point - I'll fix that in the general Encoding refactor as well

lbergelson

@jmthibault79 Looks good except for moving that buffer out of the field.

jmthibault79 · 2018-11-14T15:22:52Z

Restored buf @lbergelson

lbergelson

looks good

jmthibault79 commented Nov 7, 2018

View reviewed changes

jmthibault79 assigned lbergelson and cmnbroad Nov 7, 2018

jmthibault79 changed the title ~~Add hashCode() to classes with equals()~~ Add hashCode() to classes with equals() and clean up equals() Nov 7, 2018

lbergelson requested changes Nov 7, 2018

View reviewed changes

Let IntelliJ fix equals() and hashCode() for classes with problems

d27129e

jmthibault79 force-pushed the jt_hashCode branch from 7aa8a4a to d27129e Compare November 9, 2018 14:14

restore Version equals() comment

9a52ec6

jmthibault79 commented Nov 9, 2018

View reviewed changes

jmthibault79 added 2 commits November 9, 2018 09:25

mini cleanup

f54a9f3

revert to master

4a51397

lbergelson requested changes Nov 9, 2018

View reviewed changes

lbergelson assigned jmthibault79 and unassigned lbergelson and cmnbroad Nov 9, 2018

restore buf field

83dcb77

lbergelson approved these changes Nov 14, 2018

View reviewed changes

lbergelson merged commit ee0ad68 into master Nov 14, 2018

lbergelson deleted the jt_hashCode branch November 14, 2018 16:33

jmthibault79 mentioned this pull request Nov 29, 2018

CRAM: Refactor Encodings and Codecs #1224

Merged

5 tasks

jmthibault79 mentioned this pull request Feb 13, 2019

A few fixes for issues found by spotbugs #1278

Merged

5 tasks

cmnbroad mentioned this pull request Oct 2, 2019

CramCompressionRecord defines equals but not hashCode #1220

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add hashCode() to classes with equals() and clean up equals() #1222

Add hashCode() to classes with equals() and clean up equals() #1222

jmthibault79 commented Nov 7, 2018 •

edited

Loading

jmthibault79 commented Nov 7, 2018 •

edited

Loading

jmthibault79 Nov 7, 2018

jmthibault79 Nov 7, 2018

codecov-io commented Nov 7, 2018 •

edited

Loading

jmthibault79 Nov 7, 2018

lbergelson left a comment

lbergelson Nov 7, 2018

lbergelson Nov 7, 2018

lbergelson Nov 7, 2018

lbergelson Nov 7, 2018

lbergelson Nov 7, 2018

lbergelson Nov 7, 2018

lbergelson Nov 7, 2018

lbergelson Nov 7, 2018

lbergelson Nov 7, 2018

lbergelson Nov 7, 2018

jmthibault79 Nov 9, 2018

lbergelson Nov 9, 2018

jmthibault79 Nov 14, 2018

lbergelson left a comment

jmthibault79 commented Nov 14, 2018

lbergelson left a comment

Add hashCode() to classes with equals() and clean up equals() #1222

Add hashCode() to classes with equals() and clean up equals() #1222

Conversation

jmthibault79 commented Nov 7, 2018 • edited Loading

Description

Checklist

jmthibault79 commented Nov 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Nov 7, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

lbergelson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lbergelson left a comment

Choose a reason for hiding this comment

jmthibault79 commented Nov 14, 2018

lbergelson left a comment

Choose a reason for hiding this comment

jmthibault79 commented Nov 7, 2018 •

edited

Loading

jmthibault79 commented Nov 7, 2018 •

edited

Loading

codecov-io commented Nov 7, 2018 •

edited

Loading