samtools / htsjdk

A Java API for high-throughput sequencing data (HTS) formats.
http://samtools.github.io/htsjdk/
276 stars 244 forks source link

Histogram class uses unsafe methods for returning min/max #1657

Open rickymagner opened 1 year ago

rickymagner commented 1 year ago

Description of the issue:

Using the Histogram methods to get values when the histogram is empty are implemented in an unsafe way, leading to unhelpful error messages in some edge cases. This is related to this issue in Picard and is inspired by stumbling on that edge case in a ticket.

Your environment:

Steps to reproduce

Call getMin() or getMax() on any Histogram without any values in it.

Expected behaviour

Some helpful error should be thrown in this case so users can track down what went wrong better.

Actual behaviour

A NullPointerException is thrown.

More Details

The getMin method is implemented in one line:

public double getMin() {
        return map.firstEntry().getValue().getIdValue();
    }

However, the .firstEntry method will return null in the case mentioned above. Instead, something like

public double getMin() throws HistogramEmptyException {
        final first = map.firstEntry();
        if (first != null) {
            return first.getValue().getIdValue();
        } else {
            throw new HistogramEmtpyException;
        }
    }

Or any other paradigm that could add some safety rails/assurances the tools depending on it would better handle this case.