A few fixes for issues found by spotbugs (#1278)

* A few fixes for issues found by spotbugs * Make hashCode consistent with equals and add tests.
samtools · Feb 27, 2019 · 62388a2 · 62388a2
1 parent 0b16296
commit 62388a2
Show file tree

Hide file tree

Showing 9 changed files with 110 additions and 3 deletions.
diff --git a/src/main/java/htsjdk/samtools/cram/encoding/readfeatures/Substitution.java b/src/main/java/htsjdk/samtools/cram/encoding/readfeatures/Substitution.java
@@ -18,6 +18,7 @@
 package htsjdk.samtools.cram.encoding.readfeatures;
 
 import java.io.Serializable;
+import java.util.Objects;
 
 /**
  * A substitution event captured in read coordinates. It is characterized by position in read, read base and reference base.
@@ -116,6 +117,14 @@ public boolean equals(final Object obj) {
         return true;
     }
 
+    @Override
+    public int hashCode() {
+        if (code == NO_CODE) {
+            return Objects.hash(position);
+        }
+        return Objects.hash(position, base, referenceBase);
+    }
+
     @Override
     public String toString() {
         return String.valueOf((char) operator) + '@' + position + '\\' + (char) base + (char) referenceBase;

diff --git a/src/main/java/htsjdk/samtools/cram/structure/CramCompressionRecord.java b/src/main/java/htsjdk/samtools/cram/structure/CramCompressionRecord.java
@@ -29,6 +29,7 @@
 import java.util.Arrays;
 import java.util.Collection;
 import java.util.List;
+import java.util.Objects;
 
 public class CramCompressionRecord {
     private static final int MULTI_FRAGMENT_FLAG = 0x1;
@@ -139,6 +140,17 @@ private boolean deepEquals(final Collection<?> c1, final Collection<?> c2) {
         return (c1 == null || c1.isEmpty()) && (c2 == null || c2.isEmpty()) || c1 != null && c1.equals(c2);
     }
 
+    @Override
+    public int hashCode() {
+        int result = Objects.hash(alignmentStart, readLength, recordsToNextFragment, mappingQuality, flags, readName);
+        if (readFeatures != null && !readFeatures.isEmpty()) {
+            result = 31 * result + Objects.hash(readFeatures);
+        }
+        result = 31 * result + Arrays.hashCode(readBases);
+        result = 31 * result + Arrays.hashCode(qualityScores);
+        return result;
+    }
+
     @Override
     public String toString() {
         final StringBuilder stringBuilder = new StringBuilder("[");

diff --git a/src/main/java/htsjdk/samtools/reference/FastaSequenceIndex.java b/src/main/java/htsjdk/samtools/reference/FastaSequenceIndex.java
@@ -39,6 +39,7 @@
 import java.util.Iterator;
 import java.util.LinkedHashMap;
 import java.util.Map;
+import java.util.Objects;
 import java.util.Scanner;
 import java.util.regex.MatchResult;
 
@@ -135,6 +136,11 @@ public boolean equals(Object other) {
         return true;
     }
 
+    @Override
+    public int hashCode() {
+        return Objects.hash(sequenceEntries);
+    }
+
     /**
      * Parse the contents of an index file, caching the results internally.
      * @param in InputStream to parse.

diff --git a/src/main/java/htsjdk/samtools/util/AsyncBlockCompressedInputStream.java b/src/main/java/htsjdk/samtools/util/AsyncBlockCompressedInputStream.java
@@ -45,7 +45,7 @@
  * Note that this implementation is not synchronized. If multiple threads access an instance concurrently, it must be synchronized externally. 
  */
 public class AsyncBlockCompressedInputStream extends BlockCompressedInputStream {
-    private static final int READ_AHEAD_BUFFERS = (int)Math.ceil(Defaults.NON_ZERO_BUFFER_SIZE / BlockCompressedStreamConstants.MAX_COMPRESSED_BLOCK_SIZE);
+    private static final int READ_AHEAD_BUFFERS = (int)Math.ceil((double) Defaults.NON_ZERO_BUFFER_SIZE / BlockCompressedStreamConstants.MAX_COMPRESSED_BLOCK_SIZE);
     private static final Executor threadpool = Executors.newFixedThreadPool(Runtime.getRuntime().availableProcessors(),new ThreadFactory() {
             @Override
             public Thread newThread(Runnable r) {

diff --git a/src/main/java/htsjdk/samtools/util/CigarUtil.java b/src/main/java/htsjdk/samtools/util/CigarUtil.java
@@ -88,7 +88,7 @@ static private void elementStraddlesClippedRead(List<CigarElement> newCigar, Cig
         final CigarOperator op = c.getOperator();
         int clipAmount = clippedBases;
         if (op.consumesReadBases()){
-            if (op.consumesReferenceBases() & relativeClippedPosition > 0){
+            if (op.consumesReferenceBases() && relativeClippedPosition > 0){
                newCigar.add(new CigarElement(relativeClippedPosition, op));
             }
             if (!op.consumesReferenceBases()){

diff --git a/src/main/java/htsjdk/tribble/readers/PositionalBufferedStream.java b/src/main/java/htsjdk/tribble/readers/PositionalBufferedStream.java
@@ -167,7 +167,7 @@ public final void close() {
         try {
             is.close();
         } catch (IOException ex) {
-            new TribbleException("Failed to close PositionalBufferedStream", ex);
+            throw new TribbleException("Failed to close PositionalBufferedStream", ex);
         }
     }
 

diff --git a/src/main/java/htsjdk/variant/variantcontext/GenotypeLikelihoods.java b/src/main/java/htsjdk/variant/variantcontext/GenotypeLikelihoods.java
@@ -35,6 +35,7 @@
 import java.util.List;
 import java.util.Map;
 import java.util.HashMap;
+import java.util.Objects;
 
 public class GenotypeLikelihoods {
     private final static int NUM_LIKELIHOODS_CACHE_N_ALLELES = 5;
@@ -158,6 +159,11 @@ public String getAsString() {
         return Arrays.equals(getAsPLs(), that.getAsPLs());
     }
 
+    @Override
+    public int hashCode() {
+        return Arrays.hashCode(getAsPLs());
+    }
+
     //Return genotype likelihoods as an EnumMap with Genotypes as keys and likelihoods as values
     //Returns null in case of missing likelihoods
     public EnumMap<GenotypeType,Double> getAsMap(boolean normalizeFromLog10){

diff --git a/src/test/java/htsjdk/samtools/cram/encoding/ReadFeaturesTest.java b/src/test/java/htsjdk/samtools/cram/encoding/ReadFeaturesTest.java
@@ -4,9 +4,13 @@
 import htsjdk.samtools.cram.encoding.readfeatures.Bases;
 import htsjdk.samtools.cram.encoding.readfeatures.Scores;
 import htsjdk.samtools.cram.encoding.readfeatures.SoftClip;
+import htsjdk.samtools.cram.encoding.readfeatures.Substitution;
 import org.testng.Assert;
 import org.testng.annotations.Test;
 
+import java.util.ArrayList;
+import java.util.List;
+
 public class ReadFeaturesTest extends HtsjdkTest {
 
     // equals() was incorrect for these classes
@@ -25,4 +29,29 @@ public void faultyEquality() {
         final SoftClip sc2 = new SoftClip(0, new byte[] {});
         Assert.assertEquals(sc1, sc2);
     }
+
+    @Test
+    public void testSubstitutionEqualsAndHashCodeAreConsistent() {
+        final List<Substitution> substitutions = new ArrayList<>();
+        for (int position : new int[] {0, 1}) {
+            for (byte base : new byte[] {(byte) -1, (byte) 'A'}) {
+                for (byte referenceBase : new byte[] {(byte) -1, (byte) 'C'}) {
+                    for (byte code : new byte[] {Substitution.NO_CODE, (byte) 1, (byte) 2}) {
+                        Substitution substitution = new Substitution(position, base, referenceBase);
+                        substitution.setCode(code);
+                        substitutions.add(substitution);
+                    }
+                }
+            }
+        }
+
+        for (Substitution s1 : substitutions) {
+            for (Substitution s2 : substitutions) {
+                if (s1.equals(s2)) {
+                    Assert.assertEquals(s1.hashCode(), s2. hashCode(),
+                            String.format("Comparing %s (%s) and %s (%s)", s1, s1.getCode(), s2, s2.getCode()));
+                }
+            }
+        }
+    }
 }
diff --git a/src/test/java/htsjdk/samtools/cram/structure/CramCompressionRecordTest.java b/src/test/java/htsjdk/samtools/cram/structure/CramCompressionRecordTest.java
@@ -1,5 +1,7 @@
 package htsjdk.samtools.cram.structure;
 
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.Lists;
 import htsjdk.HtsjdkTest;
 import htsjdk.samtools.SAMRecord;
 import htsjdk.samtools.cram.encoding.readfeatures.*;
@@ -189,4 +191,47 @@ public void test_placementForAlignmentSpanAndEnd(final int sequenceId,
             Assert.assertEquals(r.getAlignmentEnd(usePositionDeltaEncoding), Slice.NO_ALIGNMENT_END);
         }
     }
+
+    @Test
+    public void testEqualsAndHashCodeAreConsistent() {
+        final List<CramCompressionRecord> records = new ArrayList<>();
+
+        final List<ReadFeature> features = new ArrayList<>();
+        String softClip = "AAA";
+        features.add(new SoftClip(1, softClip.getBytes()));
+        String insertion = "CCCCCCCCCC";
+        features.add(new Insertion(1, insertion.getBytes()));
+
+        for (int alignmentStart : new int[] {0, 1}) {
+            for (int readLength : new int[] {100, 101}) {
+                for (int flags : new int[] {0, 0x4}) {
+                    for (List<ReadFeature> readFeatures : Lists.<List<ReadFeature>>newArrayList(null, new ArrayList<>(), features)) {
+                        for (String readName : new String[] {null, "", "r"}) {
+                            for (byte[] readBases : new byte[][]{null, new byte[]{(byte) 'A', (byte) 'C'}}) {
+                                for (byte[] qualityScores : new byte[][]{null, new byte[]{(byte) 1, (byte) 2}}) {
+                                    final CramCompressionRecord r = new CramCompressionRecord();
+                                    r.alignmentStart = alignmentStart;
+                                    r.readLength = readLength;
+                                    r.flags = flags;
+                                    r.readFeatures = readFeatures;
+                                    r.readName = readName;
+                                    r.readBases = readBases;
+                                    r.qualityScores = qualityScores;
+                                    records.add(r);
+                                }
+                            }
+                        }
+                    }
+                }
+            }
+        }
+
+        for (CramCompressionRecord r1 : records) {
+            for (CramCompressionRecord r2 : records) {
+                if (r1.equals(r2)) {
+                    Assert.assertEquals(r1.hashCode(), r2.hashCode(), String.format("Comparing %s and %s", r1, r2));
+                }
+            }
+        }
+    }
 }