cleanup bam order checking code (#770)

* cleanup of code for checking order of sam files

making use of SAMSortOrderChecker in AssertingSamIterator
  this reduces some redundant code and makes order checking more consistent
removing some redundant ways of getting a Comparator from a SamFileHeader.SortOrder
replacing use of reflection to generate Comparators with use of Supplier

src/main/java/htsjdk/samtools/SAMFileHeader.java

@@ -29,16 +29,8 @@
 import htsjdk.samtools.util.Log;
 
 import java.io.StringWriter;
-import java.lang.reflect.Constructor;
-import java.util.ArrayList;
-import java.util.Arrays;
-import java.util.Collection;
-import java.util.Collections;
-import java.util.HashMap;
-import java.util.HashSet;
-import java.util.List;
-import java.util.Map;
-import java.util.Set;
+import java.util.*;
+import java.util.function.Supplier;
 
 /**
  * Header information from a SAM or BAM file.
@@ -70,40 +62,32 @@ Set<String> getStandardTags() {
      * Ways in which a SAM or BAM may be sorted.
      */
     public enum SortOrder {
-        unsorted(null),
-        queryname(SAMRecordQueryNameComparator.class),
-        coordinate(SAMRecordCoordinateComparator.class),
-        duplicate(SAMRecordDuplicateComparator.class), // NB: this is not in the SAM spec!
-        unknown(null);
+        unsorted(() -> null),
+        queryname(SAMRecordQueryNameComparator::new),
+        coordinate(SAMRecordCoordinateComparator::new),
+        duplicate(SAMRecordDuplicateComparator::new), // NB: this is not in the SAM spec!
+        unknown(() -> null);
 
-        private final Class<? extends SAMRecordComparator> comparator;
+        private final Supplier<SAMRecordComparator> comparatorSupplier;
 
-        SortOrder(final Class<? extends SAMRecordComparator> comparatorClass) {
-            this.comparator = comparatorClass;
+        SortOrder(final Supplier<SAMRecordComparator> comparatorClass) {
+            this.comparatorSupplier = comparatorClass;
         }
 
         /**
          * @return Comparator class to sort in the specified order, or null if unsorted.
+         * @deprecated since 7/2018, use {@link #getComparatorInstance()} with {@link SAMSortOrderChecker#getClass()} instead
          */
+        @Deprecated
         public Class<? extends SAMRecordComparator> getComparator() {
-            return comparator;
+            return comparatorSupplier.get().getClass();
         }
 
         /**
          * @return Comparator to sort in the specified order, or null if unsorted.
          */
         public SAMRecordComparator getComparatorInstance() {
-            if (comparator != null) {
-                try {
-                    final Constructor<? extends SAMRecordComparator> ctor = comparator.getConstructor();
-                    return ctor.newInstance();
-                }
-                catch (Exception e) {
-                    throw new IllegalStateException("Could not instantiate a comparator for sort order: " +
-                            this.name(), e);
-                }
-            }
-            return null;
+            return comparatorSupplier.get();
         }
     }
 

src/main/java/htsjdk/samtools/SAMFileWriterImpl.java

@@ -156,7 +156,7 @@ public void setHeader(final SAMFileHeader header)
             }
         } else if (!sortOrder.equals(SAMFileHeader.SortOrder.unsorted)) {
             alignmentSorter = SortingCollection.newInstance(SAMRecord.class,
-                    new BAMRecordCodec(header), makeComparator(), maxRecordsInRam, tmpDir);
+                    new BAMRecordCodec(header), sortOrder.getComparatorInstance(), maxRecordsInRam, tmpDir);
         }
     }
 
@@ -165,20 +165,6 @@ public SAMFileHeader getFileHeader() {
         return header;
     }
 
-    private SAMRecordComparator makeComparator() {
-        switch (sortOrder) {
-            case coordinate:
-                return new SAMRecordCoordinateComparator();
-            case queryname:
-                return new SAMRecordQueryNameComparator();
-            case duplicate:
-                return new SAMRecordDuplicateComparator();
-            case unsorted:
-                return null;
-        }
-        throw new IllegalStateException("sortOrder should not be null");
-    }
-
     /**
      * Add an alignment record to be emitted by the writer.
      *

src/main/java/htsjdk/samtools/SAMSortOrderChecker.java

@@ -34,21 +34,7 @@ public class SAMSortOrderChecker {
 
     public SAMSortOrderChecker(final SAMFileHeader.SortOrder sortOrder) {
         this.sortOrder = sortOrder;
-        switch (sortOrder) {
-            case coordinate:
-                comparator = new SAMRecordCoordinateComparator();
-                break;
-            case queryname:
-                comparator = new SAMRecordQueryNameComparator();
-                break;
-            case duplicate:
-                comparator = new SAMRecordDuplicateComparator();
-                break;
-            case unsorted:
-            default:
-                comparator = null;
-                break;
-        }
+        this.comparator = sortOrder == null ? null : sortOrder.getComparatorInstance();
     }
 
     /**
@@ -71,19 +57,22 @@ public SAMRecord getPreviousRecord() {
         return prev;
     }
 
+    public SAMFileHeader.SortOrder getSortOrder() {
+        return sortOrder;
+    }
+
     /**
      * Return the sort key used for the given sort order.  Useful in error messages.
+     * This should only be used in error messages and program correctness should not rely on this.
      */
     public String getSortKey(final SAMRecord rec) {
         switch (sortOrder) {
-
             case coordinate:
                 return rec.getReferenceName() + ":" + rec.getAlignmentStart();
             case queryname:
                 return rec.getReadName();
-            case unsorted:
             default:
-                return null;
+                return rec.getSAMString().trim();
         }
     }
 }

src/main/java/htsjdk/samtools/SamReader.java

@@ -552,44 +552,29 @@ static AssertingIterator of(final CloseableIterator<SAMRecord> iterator) {
         }
 
         private final CloseableIterator<SAMRecord> wrappedIterator;
-        private SAMRecord previous = null;
-        private SAMRecordComparator comparator = null;
+        private SAMSortOrderChecker checker;
 
         public AssertingIterator(final CloseableIterator<SAMRecord> iterator) {
             wrappedIterator = iterator;
         }
 
         @Override
         public SAMRecordIterator assertSorted(final SAMFileHeader.SortOrder sortOrder) {
-
-            if (sortOrder == null || sortOrder == SAMFileHeader.SortOrder.unsorted) {
-                comparator = null;
-                return this;
-            }
-
-            comparator = sortOrder.getComparatorInstance();
+            checker = new SAMSortOrderChecker(sortOrder);
             return this;
         }
 
         @Override
         public SAMRecord next() {
             final SAMRecord result = wrappedIterator.next();
-            if (comparator != null) {
-                if (previous != null) {
-                    if (comparator.fileOrderCompare(previous, result) > 0) {
-                        throw new IllegalStateException(MessageFormat.format(
-                                "Records {0} ({1}:{2}) should come after {3} ({4}:{5}) when sorting with {6}",
-                                previous.getReadName(),
-                                previous.getReferenceName(),
-                                previous.getAlignmentStart(),
-                                result.getReadName(),
-                                result.getReferenceName(),
-                                result.getAlignmentStart(),
-                                comparator.getClass().getName())
-                        );
-                    }
+            if (checker != null) {
+                final SAMRecord previous = checker.getPreviousRecord();
+                if (!checker.isSorted(result)) {
+                    throw new IllegalStateException(String.format(
+                            "Record %s should come after %s when sorting with %s ordering.",
+                            previous.getSAMString().trim(),
+                            result.getSAMString().trim(), checker.getSortOrder()));
                 }
-                previous = result;
             }
             return result;
         }