fix CRAMIterator when next() is called without hasNext() #1193

lbergelson · 2018-10-10T21:56:54Z

fixing a bug in CRAMIterator which previously threw if next() was called without first calling hasNext()
adding CRAMTestUtils and extracting a method to it from CramEdgeCasesTest

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)

codecov-io · 2018-10-10T22:10:23Z

Codecov Report

Merging #1193 into master will increase coverage by 0.018%.
The diff coverage is 75%.

@@               Coverage Diff               @@
##              master     #1193       +/-   ##
===============================================
+ Coverage     68.405%   68.423%   +0.018%     
- Complexity      8017      8020        +3     
===============================================
  Files            542       542               
  Lines          32701     32701               
  Branches        5530      5530               
===============================================
+ Hits           22369     22375        +6     
+ Misses          8127      8123        -4     
+ Partials        2205      2203        -2

Impacted Files	Coverage Δ	Complexity Δ
src/main/java/htsjdk/samtools/CRAMIterator.java	`78.767% <75%> (+1.37%)`	`33 <2> (+1)`	⬆️
...c/main/java/htsjdk/samtools/SAMFileWriterImpl.java	`81.944% <0%> (+1.389%)`	`25% <0%> (+1%)`	⬆️
...samtools/util/AsyncBlockCompressedInputStream.java	`76% <0%> (+4%)`	`13% <0%> (+1%)`	⬆️

pshapiro4broad

Some minor things

pshapiro4broad · 2018-10-11T14:22:30Z

src/main/java/htsjdk/samtools/CRAMIterator.java

@@ -271,7 +266,11 @@ public boolean hasNext() {

    @Override
    public SAMRecord next() {
-        return iterator.next();
+        if(hasNext()) {


missing space after if

pshapiro4broad · 2018-10-11T14:23:32Z

src/test/java/htsjdk/samtools/CRAMTestUtils.java

+
+public final class CRAMTestUtils {
+
+    private CRAMTestUtils(){};


I understand that there's no point in instantiating this class, but why bother enforcing it with a private constructor?

It does make it clear that there is no point in instantiating the class, which I like.

Unlike a singleton, there's no harm if a user decides to instantiate it. They can't break anything or change behavior that we'd like to control. So I'd still say there is no need to bother enforcing it for this class. But I guess it comes down to a matter of taste..

pshapiro4broad · 2018-10-11T14:25:02Z

src/test/java/htsjdk/samtools/CRAMTestUtils.java

+
+    private static CRAMFileReader writeAndReadFromInMemoryCram(Collection<SAMRecord> records, CRAMReferenceSource source, SAMFileHeader header) throws IOException {
+        ByteArrayOutputStream baos = new ByteArrayOutputStream();
+        CRAMFileWriter cramFileWriter = new CRAMFileWriter(baos, source, header, "whatever");


Use try-with-resources to avoid close() below. Since baos is closable too you could put that in the try as well.

I stuck it in a try with resources so that it will always close on exit, but I left the close to force a flush before reading from the buffer, the alternative was a gross nested try statement.

pshapiro4broad · 2018-10-11T14:26:54Z

src/test/java/htsjdk/samtools/CRAMTestUtils.java

+    /**
+     * return a CRAMReferenceSource that returns all A's for any sequence queried
+     */
+    public static CRAMReferenceSource getFakeReferenceSource(){


missing space between ){

cmnbroad

Couple of minor comments. Thx for adding the utilities.

cmnbroad · 2018-10-11T20:30:54Z

src/test/java/htsjdk/samtools/CRAMFileReaderTest.java

+        final CRAMFileReader reader = CRAMTestUtils.writeAndReadFromInMemoryCram(builder);
+        final SAMRecordIterator iterator = reader.getIterator();
+        Assert.assertNotNull(iterator.next());
+        Assert.assertThrows(NoSuchElementException.class, iterator::next);


That two tests, but, ok...

cmnbroad · 2018-10-11T20:32:13Z

src/test/java/htsjdk/samtools/CRAMTestUtils.java

+
+public final class CRAMTestUtils {
+
+    private CRAMTestUtils(){};


It does make it clear that there is no point in instantiating the class, which I like.

* fixing a bug in CRAMIterator which previously threw if next() was called without first calling hasNext() * adding CRAMTestUtils and extracting a method to it from CramEdgeCasesTest

lbergelson assigned cmnbroad Oct 10, 2018

lbergelson requested a review from cmnbroad October 10, 2018 21:56

pshapiro4broad reviewed Oct 11, 2018

View reviewed changes

cmnbroad approved these changes Oct 11, 2018

View reviewed changes

lbergelson added 2 commits October 15, 2018 17:28

fix CRAMIterator when next() is called without hasNext()

4e6f6d7

* fixing a bug in CRAMIterator which previously threw if next() was called without first calling hasNext() * adding CRAMTestUtils and extracting a method to it from CramEdgeCasesTest

responding to comments

3fb463d

lbergelson force-pushed the lb_fix_CramIterator_next branch from 39c0997 to 3fb463d Compare October 15, 2018 21:29

lbergelson merged commit d504256 into master Oct 16, 2018

lbergelson deleted the lb_fix_CramIterator_next branch October 16, 2018 18:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix CRAMIterator when next() is called without hasNext() #1193

fix CRAMIterator when next() is called without hasNext() #1193

lbergelson commented Oct 10, 2018

codecov-io commented Oct 10, 2018 •

edited

Loading

pshapiro4broad left a comment

pshapiro4broad Oct 11, 2018

pshapiro4broad Oct 11, 2018

cmnbroad Oct 11, 2018

pshapiro4broad Oct 16, 2018 •

edited

Loading

pshapiro4broad Oct 11, 2018

lbergelson Oct 15, 2018

pshapiro4broad Oct 11, 2018

cmnbroad left a comment

cmnbroad Oct 11, 2018

cmnbroad Oct 11, 2018


		public final class CRAMTestUtils {

		private CRAMTestUtils(){};

fix CRAMIterator when next() is called without hasNext() #1193

fix CRAMIterator when next() is called without hasNext() #1193

Conversation

lbergelson commented Oct 10, 2018

Checklist

codecov-io commented Oct 10, 2018 • edited Loading

Codecov Report

pshapiro4broad 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

pshapiro4broad Oct 16, 2018 • edited Loading

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

cmnbroad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Oct 10, 2018 •

edited

Loading

pshapiro4broad Oct 16, 2018 •

edited

Loading