4
\$\begingroup\$

Introduction

I'm working on a Statistics Library that can record observations and produce a Summary of the statistics when there's enough observations. Right now, it's at its early stages and is expected to change a lot as I develop it gradually.

Minimal Example

Suppose I want to record the run time of a particular piece of code and generate some of the common Statistics about it. At the current state of my project, the following approach is the way to go:

        SampleBuilder builder = new SampleBuilder("Runtimes", "s");

        for (int i = 0; i < ITERATION_LIMIT; i++) {
            long timeStart = System.currentTimeMillis();

            // The code to be timed goes here.

            new BigInteger(1024, 2048, new Random());

            long timeEnd = System.currentTimeMillis();

            builder.addObservation((timeEnd - timeStart) / 1000.0);
        }

        Sample runtimes = builder.buildSample();
        System.out.println(runtimes);

The output is something like:

0.121 s
0.147 s
0.22 s
0.115 s
0.084 s
0.025 s
0.353 s
0.209 s
0.021 s
0.223 s
...
0.015 s
0.521 s
0.03 s
0.017 s
0.036 s

Summary Statistics for Sample: Runtimes

Count   :    50 s
Sum     : 4.757 s
Mean    : 0.095 s
Variance: 0.011 s
Std Dev : 0.106 s

The Code

Sample.java (and Summary.java)

package in.hungrybluedev.statistics;

import java.util.LinkedHashMap;
import java.util.Map;

/**
 * A Sample is an immutable collection of observations. There are statistics
 * associated with every Sample, such as mean, variance and standard
 * deviation. This class implements various methods to access these
 * desired statistics and also presents them in an organised manner
 * through the Summary subclass.
 * <p>
 * Note that the minimum number of elements in a Sample must be {@value #DEFAULT_THRESHOLD}
 * (or whatever the threshold is set to). This is because the formulas
 * implemented do not adjust for the decrease in degree of freedom.
 *
 * @author Subhomoy Haldar (@HungryBlueDev)
 * @noinspection WeakerAccess, unused
 */
public class Sample {
    /* Statically accessible parameters */

    /**
     * The threshold is the minimum number of elements that a Sample
     * must have to ensure accurate statistical analysis. The implementation
     * allows for some flexibility by providing methods to change the
     * threshold. However, some minimum restrictions apply.
     */
    public static final int DEFAULT_THRESHOLD = 40;

    /**
     * This is the minimum threshold value allowed for the Sample size. Any
     * lower, and we cannot guarantee the accuracy of the Summary Statistics
     * generated.
     */
    public static final int MINIMUM_THRESHOLD = 30;

    private static int threshold = DEFAULT_THRESHOLD;

    /**
     * Update the minimum sample size to the new minimum value. Note that
     * the new minimum must be at least {@value MINIMUM_THRESHOLD}.
     *
     * @param newThreshold The new proposed threshold value.
     * @throws IllegalArgumentException If the value is lesser than the minimum acceptable value.
     */
    public static void setThreshold(final int newThreshold)
            throws IllegalArgumentException {
        if (newThreshold < MINIMUM_THRESHOLD) {
            throw new IllegalArgumentException("Value of threshold is too low.");
        }
        threshold = newThreshold;
    }

    /**
     * @return The current threshold value.
     */
    public static int getThreshold() {
        return threshold;
    }

    /**
     * The precision is the number of significant digits that are accurately portrayed
     * in the results. The default precision is chosen arbitrarily. Users are
     * recommended to set the precision (within the capacity of double) to change
     * the way the summary is generated.
     */
    public static final int DEFAULT_PRECISION = 3;

    private static int precision = DEFAULT_PRECISION;

    /**
     * Update the precision value to the new proposed value, provided that the
     * String format method and the size of double support it.
     *
     * @param newPrecision The proposed value to the set the precision to.
     * @throws IllegalArgumentException If the new precision is not valid.
     */
    public static void setPrecision(final int newPrecision)
            throws IllegalArgumentException {
        if (newPrecision < 1 || newPrecision > 15) {
            throw new IllegalArgumentException("Invalid precision value");
        }
        precision = newPrecision;
    }

    /**
     * @return The current precision.
     */
    public static int getPrecision() {
        return precision;
    }

    /*
     * These are helper methods that make use of the precision value to
     * determine the output of the statistics generated.
     */

    private static String format(final double value) {
        return String.format("%." + precision + "f", value);
    }

    private static String format(final int value) {
        return String.valueOf(value);
    }

    private static boolean stringIsEmpty(final String text) {
        return text == null || text.isEmpty();
    }

    // Immutable fields
    private final String name;
    private final String unit;
    private final double[] values;

    // Lazily initialized fields:
    private Summary summary;
    private Double sum;
    private Double squaredSum;

    /**
     * This is the constructor for Sample and requires a name and the array
     * of observations for the sample. Unit of measurement is optional.
     * It is recommended to use a SampleBuilder to generate a Sample.
     * If the name is not provided, the default result of super.toString() is used.
     * <p>
     * Note: It is important to ensure that the observations are equal to or
     * greater than the threshold value. See {@linkplain #getThreshold()} and
     * {@link #DEFAULT_THRESHOLD}
     *
     * @param name         The name of the sample.
     * @param unit         The unit of measurement for all
     * @param observations The array of observations in the sample.
     * @throws IllegalArgumentException If any of the parameters are empty, or invalid.
     */
    Sample(final String name, final String unit, double[] observations)
            throws IllegalArgumentException {

        if (observations == null) {
            throw new IllegalArgumentException("Empty value array");
        }

        if (observations.length < threshold) {
            throw new IllegalArgumentException(
                    "The sample size is the less than the threshold: "
                            + observations.length + " < " + threshold
            );
        }

        this.name = stringIsEmpty(name) ? super.toString() : name;
        this.unit = stringIsEmpty(unit) ? "" : unit;
        this.values = observations;

        summary = null;
        sum = null;
        squaredSum = null;
    }

    /**
     * @return The name of the Series (if non-empty).
     */
    public String getName() {
        return name;
    }

    /**
     * @return The unit of measurement for the Observations (if non-empty).
     */
    public String getUnit() {
        return unit;
    }

    /**
     * Generates the summary statistics for the Sample. For a regular Sample, the
     * guaranteed statistics calculated are:
     * <ol>
     *     <li>Count</li>
     *     <li>Sum</li>
     *     <li>Mean</li>
     *     <li>Variance</li>
     *     <li>Standard Deviation</li>
     * </ol>
     * The Summary is generated lazily. Therefore, the getSummary() function can be
     * used more than once without any degradation of performance. In fact, the
     * {@linkplain #toString()} method relies on this.
     *
     * @return A Sample.Summary object with at least the guaranteed statistics mentioned.
     */
    public Summary getSummary() {
        if (summary != null) {
            return summary;
        }
        // Add the basic summary statistics that are generally required.
        // These do not require the Sample observations to be sorted.
        summary = new Summary(this);
        summary.addStatistic("Count", format(getCount()));
        summary.addStatistic("Sum", format(getSum()));
        summary.addStatistic("Mean", format(getMean()));
        summary.addStatistic("Variance", format(getVariance()));
        summary.addStatistic("Std Dev", format(getStdDev()));

        return summary;
    }

    /**
     * @return The size of this Sample, or the number of Observations in this Sample.
     */
    public int getCount() {
        return values.length;
    }

    /**
     * @return The sum of all the observations in the Sample.
     */
    public double getSum() {
        if (sum != null) {
            return sum;
        }
        double value = 0;
        for (double item : values) {
            value += item;
        }
        sum = value;
        return value;
    }

    /**
     * @return The arithmetic mean (average) of all the observations in the Sample.
     */
    public double getMean() {
        return getSum() / getCount();
    }

    /**
     * The variance in the sample. It is calculated in a manner like (but not exactly):
     *
     * <pre>
     *     <code>
     *          double mean = // mean of observations
     *          double result = 0;
     *          for (double obs: observations) {
     *              result += Math.pow(obs - mean, 2);
     *          }
     *          return result / count;
     *     </code>
     * </pre>
     * <p>
     * Mathematically, {@code var X = sum((x - avg)^2)} where X is the sample, x is an individual
     * observation, avg is the arithmetic mean.
     * <p>
     * NOTE: There is no adjustment made for the decrease in degree of freedom. The effect
     * should be negligible because the Sample size is appropriate (at least {@value #MINIMUM_THRESHOLD}.
     *
     * @return Returns the variance of the all the observations in the Sample.
     */
    public double getVariance() {
        if (squaredSum != null) {
            return squaredSum / getCount();
        }

        double mu = getMean();
        double accumulator = 0;

        for (double item : values) {
            accumulator += Math.pow(item - mu, 2);
        }

        squaredSum = accumulator;
        return accumulator / getCount();
    }

    /**
     * The Standard Deviation in the Sample. It can be usually calculated in the following manner:
     *
     * <pre>
     *     <code>
     *         double mean = // mean of observations
     *         double result = 0;
     *         for (double obs: observations) {
     *             result += Math.pow(obs - mean, 2);
     *         }
     *         return Math.sqrt(result / count);
     *     </code>
     * </pre>
     * <p>
     * Mathematically, it is the square root of the variation. It is advantageous (and often
     * referred to as the Standard Error) because it has the same units as the observations.
     * <p>
     * NOTE: The formula is not adjusted for the decrease in degree of freedoms. However,
     * it should not make a significant difference because the Sample size is guaranteed
     * to be at least {@value MINIMUM_THRESHOLD}.
     *
     * @return The standard deviation of all the observations in the Sample.
     */
    public double getStdDev() {
        return Math.sqrt(getVariance());
    }

    public String toString() {
        StringBuilder builder = new StringBuilder();
        String unitString = stringIsEmpty(unit) ? "" : " " + unit;

        for (double observation : values) {
            builder.append(observation)
                    .append(unitString)
                    .append("\n");

        }

        return builder.toString() + "\n" + getSummary().toString();
    }

    /**
     * A Summary is a collection of Statistics and their corresponding values. Internally
     * it is a {@link Map} that stores the name of the Statistics as keys and the value
     * of that Statistic as the value in the Key-Value pair.
     */
    static class Summary {
        private final Sample owner;
        private final Map<String, String> map;

        private int maxKeyLength;
        private int maxValueLength;

        /**
         * Create a new Summary. Statistics and their values (as Strings) can be
         * added using the {@link #addStatistic(String, String)} method.
         *
         * @param owner The Sample for which this Summary contains Statistics.
         */
        Summary(final Sample owner) {
            this.owner = owner;
            map = new LinkedHashMap<>();
            maxKeyLength = 0;
            maxValueLength = 0;
        }

        /**
         * Adds a statistics and its corresponding value to this Summary.
         *
         * @param statistic The statistic (like mean, median, etc).
         * @param value     The value corresponding to the statistic.
         */
        void addStatistic(final String statistic, final String value) {
            String unit = owner.getUnit();

            String updatedValue = unit.isEmpty() || statistic.equals("Count")
                    ? value
                    : value + " " + unit;

            map.putIfAbsent(statistic, updatedValue);
            maxKeyLength = Math.max(maxKeyLength, statistic.length());
            maxValueLength = Math.max(maxValueLength, updatedValue.length());
        }

        /**
         * @param statistic The statistic whose value is sought.
         * @return The value of the statistic if it is saved in this Summary, or "Unknown".
         */
        String getStatistic(final String statistic) {
            return map.getOrDefault(statistic, "Unknown");
        }


        private static String fitString(final String text, final int width, final boolean left) {
            final String prefix = left ? "%-" : "%";
            return String.format(prefix + width + "s", text);
        }

        /**
         * @return A String form of the Statistics added to this summary and their values.
         */
        @Override
        public String toString() {
            StringBuilder builder = new StringBuilder();

            builder.append("Summary Statistics for Sample: ")
                    .append(owner.name)
                    .append("\n\n");

            for (Map.Entry<String, String> entry : map.entrySet()) {
                builder.append(fitString(entry.getKey(), maxKeyLength, true))
                        .append(": ")
                        .append(fitString(entry.getValue(), maxValueLength, false))
                        .append("\n");
            }

            return builder.toString();
        }
    }
}

SampleBuilder.java

package in.hungrybluedev.statistics;

import java.util.ArrayList;
import java.util.Collection;
import java.util.List;
import java.util.Objects;

/**
 * A Utility class for easy construction of Samples. Observations
 * can be added one by one, or all at once. Once a sufficient number
 * of observations are entered (see {@linkplain Sample#getThreshold()}
 * a sample can be generated.
 * <p>
 * Generating a sample does not invalidate the previous observations.
 * More entries can be added and the consequent samples will contain
 * all the observations (new as well as old ones). In such a scenario,
 * it is recommended to use the {@linkplain #setName(String)} method
 * to create Sample with different names (for the sake of easy organization).
 *
 * @author Subhomoy Haldar (@HungryBlueDev)
 * @noinspection unused, WeakerAccess, UnusedReturnValue
 */
public class SampleBuilder {
    private final List<Double> observationList;
    private String name;
    private String unit;

    /**
     * Default constructor that creates an internal storage list of
     * default size and the name of the sample is set to {@code null}.
     * If you want to change the name of the Sample, use the
     * {@linkplain #setName(String)} method.
     *
     * @see Sample#DEFAULT_THRESHOLD
     * @see Sample#getThreshold()
     */
    public SampleBuilder() {
        observationList = new ArrayList<>(Sample.getThreshold());
        name = null;
        unit = null;
    }

    /**
     * Creates a SampleBuilder and sets the (current) name of the
     * Sample to the proposed value. The internal list is of the default size.
     *
     * @param name The desired name of the Sample.
     * @see Sample#DEFAULT_THRESHOLD
     * @see Sample#getThreshold()
     */
    public SampleBuilder(final String name) {
        observationList = new ArrayList<>(Sample.getThreshold());
        this.name = name;
    }

    /**
     * Creates a SampleBuilder and sets the (current) name of the
     * Sample to the proposed value. The internal list is of the default size.
     *
     * @param name The desired name of the Sample.
     * @param unit The unit of measurement for all the observations in the Sample.
     * @see Sample#DEFAULT_THRESHOLD
     * @see Sample#getThreshold()
     */
    public SampleBuilder(final String name, final String unit) {
        observationList = new ArrayList<>(Sample.getThreshold());
        this.name = name;
        this.unit = unit;
    }

    /**
     * Constructor that takes the desired name of the Sample as well
     * as the tentative size of the Sample. This ensures that the value
     * of count is at least equal to the threshold value.
     *
     * @param name  The desired name of the Sample.
     * @param unit  The unit of measurement for all the observations in the Sample.
     * @param count The proposed size of the Sample.
     * @throws IllegalArgumentException If the count is lower than the threshold.
     * @see Sample#DEFAULT_THRESHOLD
     * @see Sample#getThreshold()
     */
    public SampleBuilder(final String name, final String unit, final int count) {
        if (count < Sample.getThreshold()) {
            throw new IllegalArgumentException("The count is lower than the threshold.");
        }
        observationList = new ArrayList<>(count);
        this.name = name;
        this.unit = unit;
    }

    /**
     * Sets the name of the Sample being built to the proposed value.
     *
     * @param name The name to be given to the next Sample that is generated.
     * @return This SampleBuilder to facilitate chaining of method calls.
     */
    public SampleBuilder setName(final String name) {
        this.name = name;
        return this;
    }

    /**
     * Sets the unit of measurement for the observations in the Sample.
     *
     * @param unit The unit of measurement for all the observations in the Sample.
     * @return This SampleBuilder to facilitate chaining of method calls.
     */
    public SampleBuilder setUnit(final String unit) {
        this.unit = unit;
        return this;
    }

    /**
     * Adds an observation value to the list. The requirement is that the
     * value must be finite (not infinite or NaN).
     *
     * @param observation The observation to add to the current Sample.
     * @return This SampleBuilder to facilitate chaining of method calls.
     * @throws IllegalArgumentException If the observation is not finite.
     * @see Double#isFinite(double)
     */
    public SampleBuilder addObservation(final double observation)
            throws IllegalArgumentException {

        if (!Double.isFinite(observation)) {
            throw new IllegalArgumentException("Observations must be finite.");
        }

        observationList.add(observation);
        return this;
    }

    /**
     * Adds a chunk of observations to the current Sample. The criterion for individual
     * observations holds: only finite values (no infinite or NaN).
     *
     * @param observationChunk The chunk of observations to add to the current Sample.
     * @return This SampleBuilder to facilitate chaining of method calls.
     * @throws IllegalArgumentException If any of the observations are not finite.
     */
    public SampleBuilder addObservations(final double[] observationChunk)
            throws IllegalArgumentException {
        for (double observation : observationChunk) {
            addObservation(observation);
        }
        return this;
    }

    /**
     * Adds a {@link Collection} of observations to the current Sample. The criteria
     * for valid observations include:
     * <ol>
     *     <li>Must be non-null</li>
     *     <li>Must be finite (not infinite or NaN)</li>
     * </ol>
     *
     * @param observationsCollection The collections of elements to be added.
     * @return This SampleBuilder to facilitate chaining of method calls.
     * @throws IllegalArgumentException If any of the items is null, infinite or NaN.
     */
    public SampleBuilder addObservations(final Collection<Double> observationsCollection)
            throws IllegalArgumentException {
        for (Double observation : observationsCollection) {
            Objects.requireNonNull(observation);
            addObservation(observation);
        }
        return this;
    }

    /**
     * @return The current count of observations in the internal list.
     */
    public int getCount() {
        return observationList.size();
    }

    /**
     * @return A Sample instance from the observations collected thus far.
     * @throws IllegalStateException If the sample size does not exceed the minimum required threshold.
     * @see Sample#DEFAULT_THRESHOLD
     * @see Sample#getThreshold()
     */
    public Sample buildSample()
            throws IllegalStateException {

        final int count = getCount();
        if (count < Sample.getThreshold()) {
            throw new IllegalStateException("The sample does not contain enough observations.");
        }

        Double[] rawOutput = observationList.toArray(new Double[count]);
        double[] observations = new double[count];

        for (int i = 0; i < count; i++) {
            observations[i] = rawOutput[i];
        }

        return new Sample(name, unit, observations);
    }
}

SampleTest.java

package in.hungrybluedev.statistics;

import java.util.Arrays;
import java.util.Random;

import static org.testng.Assert.assertEquals;
import static org.testng.Assert.assertThrows;

public class SampleTest {

    private static final int TEST_COUNT = 100;
    private static final int RANDOM_MAX = 1000;
    private static final double EPSILON = 1e-10;

    private static Random random = new Random();

    private static int getRandomSampleSize() {
        return random.nextInt(RANDOM_MAX) + Sample.getThreshold();
    }

    /**
     * Implementation of the classic Fischer-Yates shuffle algorithm.
     *
     * @param observation The array of observations to be shuffled.
     */
    private static void shuffle(final double[] observation) {
        for (int i = observation.length - 1; i >= 1; i--) {
            int j = random.nextInt(i);
            swap(observation, i, j);
        }
    }

    private static void swap(double[] observation, int i, int j) {
        double temp = observation[i];
        observation[i] = observation[j];
        observation[j] = temp;
    }

    @org.testng.annotations.Test
    public void testThreshold() {
        int[] testThresholds = {30, 20, 10, 40, 100, 2};

        for (int threshold : testThresholds) {

            // The point is to ensure that the sample size is never below
            // the minimum threshold. If it is lower, then an exception
            // should be thrown. Otherwise everything should work.
            if (threshold < Sample.MINIMUM_THRESHOLD) {

                assertThrows(IllegalArgumentException.class, () -> Sample.setThreshold(threshold));

            } else {

                Sample.setThreshold(threshold);
                assertEquals(Sample.getThreshold(), threshold);

            }
        }

        Sample.setThreshold(Sample.DEFAULT_THRESHOLD);
    }

    @org.testng.annotations.Test
    public void testGetSummary() {
        String expected = "Summary Statistics for Sample: Test sample\n" +
                "\n" +
                "Count   :          50\n" +
                "Sum     : 1225.000 km\n" +
                "Mean    :   24.500 km\n" +
                "Variance:  208.250 km\n" +
                "Std Dev :   14.431 km\n";

        SampleBuilder builder = new SampleBuilder("Test sample", "km");

        for (int i = 0; i < 50; i++) {
            builder.addObservation(i);
        }

        Sample sample = builder.buildSample();

        assertEquals(sample.getSummary().toString(), expected);
    }

    @org.testng.annotations.Test
    public void testGetCount() {
        for (int i = 1; i <= TEST_COUNT; i++) {
            final int n = getRandomSampleSize();
            final double[] observations = new double[n];

            Sample sample = new Sample("Sample number " + i, null, observations);
            assertEquals(sample.getCount(), n);
        }
    }

    @org.testng.annotations.Test
    public void testGetSum() {
        for (int i = 1; i <= TEST_COUNT; i++) {
            final int n = getRandomSampleSize();
            final double[] observations = new double[n];

            for (int j = 0; j < n; j++) {
                observations[j] = (j + 1);
            }

            shuffle(observations);

            Sample sample = new Sample("Sample number " + i, "cm", observations);
            assertEquals(sample.getSum(), n * (n + 1.0) / 2, EPSILON);
        }
    }

    @org.testng.annotations.Test
    public void testGetMean() {
        for (int i = 1; i <= TEST_COUNT; i++) {
            final int n = getRandomSampleSize();
            final double[] observations = new double[n];

            for (int j = 0; j < n; j++) {
                observations[j] = (j + 1);
            }

            shuffle(observations);

            Sample sample = new Sample("Sample number " + i, "A", observations);
            assertEquals(sample.getMean(), (n + 1.0) / 2, EPSILON);
        }
    }

    @org.testng.annotations.Test
    public void testZeroVariance() {
        for (int i = 1; i <= TEST_COUNT; i++) {
            final int n = getRandomSampleSize();
            final double[] observations = new double[n];
            final double constant = random.nextInt(RANDOM_MAX);

            Arrays.fill(observations, constant);

            Sample sample = new Sample("Sample number " + i, "J", observations);
            assertEquals(sample.getVariance(), 0, EPSILON);
            assertEquals(sample.getStdDev(), 0, EPSILON);
        }
    }

    @org.testng.annotations.Test
    public void testStdDev() {
        for (int i = 1; i <= TEST_COUNT; i++) {
            final int n = getRandomSampleSize() * 2;
            final double[] observations = new double[n];

            double mean = random.nextDouble();
            double error = random.nextDouble();

            int factor = -1;

            for (int j = 0; j < n; j++) {
                observations[j] = mean + error * factor;
                factor *= -1;
            }

            Sample sample = new Sample("Gaussian Sample #" + i, null, observations);
            assertEquals(sample.getMean(), mean, EPSILON);
            assertEquals(sample.getStdDev(), error, EPSILON);
        }
    }

    @org.testng.annotations.Test
    public void testTestSampleToString() {
        String expectedResult = "0.0 km\n" +
                "1.0 km\n" +
                "2.0 km\n" +
                "3.0 km\n" +
                "4.0 km\n" +
                "5.0 km\n" +
                "6.0 km\n" +
                "7.0 km\n" +
                "8.0 km\n" +
                "9.0 km\n" +
                "10.0 km\n" +
                "11.0 km\n" +
                "12.0 km\n" +
                "13.0 km\n" +
                "14.0 km\n" +
                "15.0 km\n" +
                "16.0 km\n" +
                "17.0 km\n" +
                "18.0 km\n" +
                "19.0 km\n" +
                "20.0 km\n" +
                "21.0 km\n" +
                "22.0 km\n" +
                "23.0 km\n" +
                "24.0 km\n" +
                "25.0 km\n" +
                "26.0 km\n" +
                "27.0 km\n" +
                "28.0 km\n" +
                "29.0 km\n" +
                "30.0 km\n" +
                "31.0 km\n" +
                "32.0 km\n" +
                "33.0 km\n" +
                "34.0 km\n" +
                "35.0 km\n" +
                "36.0 km\n" +
                "37.0 km\n" +
                "38.0 km\n" +
                "39.0 km\n" +
                "40.0 km\n" +
                "41.0 km\n" +
                "42.0 km\n" +
                "43.0 km\n" +
                "44.0 km\n" +
                "45.0 km\n" +
                "46.0 km\n" +
                "47.0 km\n" +
                "48.0 km\n" +
                "49.0 km\n" +
                "\n" +
                "Summary Statistics for Sample: Test sample\n" +
                "\n" +
                "Count   :          50\n" +
                "Sum     : 1225.000 km\n" +
                "Mean    :   24.500 km\n" +
                "Variance:  208.250 km\n" +
                "Std Dev :   14.431 km\n";

        SampleBuilder builder = new SampleBuilder("Test sample", "km");

        for (int i = 0; i < 50; i++) {
            builder.addObservation(i);
        }

        Sample sample = builder.buildSample();

        assertEquals(sample.toString(), expectedResult);
    }
}

Specific Requests

  1. You are free to critique any part of the code. Don't hold back.
  2. At this stage, the library is (obviously) feature incomplete. What could I add to extend its capabilities?
  3. Are there any long-term concerns that might require significant alterations to fix? If so, I'd like to consider them sooner rather than later.
  4. Any unit tests that are a must? Or would be nice to have?
\$\endgroup\$
4
\$\begingroup\$

I'm concentrating only on the API design.

final long timeStart = System.currentTimeMillis();
...
final long timeEnd = System.currentTimeMillis();
builder.addObservation((timeEnd - timeStart) / 1000.0);

This code is IMHO a bit too cumbersome for the user and allows for programming errors to mess up the statistics. The user has to remember to do the logging exactly the same way every time. I would like to see the time keeping offloaded to the statistics library itself.

For example with an observation token that has an internal callback to the builder. This would have the benefit that if you don't want to gather statistics, you could control it with a config switch and just return a common dummy token that does nothing (zero memory footprint, practically no processor time wasted).

final Observation observation = builder.startObservation();
...
observation.finish();

Possibly with a Runnable:

builder.observe(() -> {
    ...
});

Once this is implemented, the recording of the time stamps should be made with a Supplier<Long> if the user wants to log a specific time source other than System.currentTimeMillis(). This feature would also allow efficient unit testing.

private Supplier<Long> timeSupplier = () -> System.currentTimeMillis();

public void setTimeSupplier(Supplier<Long> timeSupplier) {
    this.timeSupplier = timeSupplier;
}

Instead of fractions of seconds as doubles I would just log the smallest available time units (millis) and convert them to human readable format during formatting.

| improve this answer | |
\$\endgroup\$
  • 1
    \$\begingroup\$ I think that 'time' was really just an example of how the library might be used. It could be used for other things, such as transaction amount, distance ran etc, which is why one of the tests seems to do some analysis on km. \$\endgroup\$ – forsvarir Feb 28 at 14:22
  • \$\begingroup\$ I think the custom mechanism for easy runtime measurement is definitely very helpful. However, @forsvarir is right because I want my library to be as general as possible. I might implement your suggestions in a separate class or package though; it's too cool to ignore. \$\endgroup\$ – Hungry Blue Dev Feb 29 at 6:58
  • \$\begingroup\$ The Supplier<Long> can return anything, not just time. And it can be a double instead of long if you think it's more flexible. \$\endgroup\$ – TorbenPutkonen Feb 29 at 21:28
3
\$\begingroup\$

I have some suggestions for you.

Code Review

Sample class

In my opinion, the variables and the constants declarations should be moved to the top of the class / block.

format(double) method

Instead of using the java.lang.String#format to format the double value, I suggest that you use the java.text.NumberFormat. This class will allow you to set the required precision by calling the java.text.NumberFormat#setMaximumFractionDigits method.

double value = 0.123456;
int precision = 3;

NumberFormat nf = NumberFormat.getInstance();
nf.setMaximumFractionDigits(precision);


System.out.println(nf.format(value));
System.out.println(String.format("%." + precision + "f", value));

getSum, getMean, getVariance and getStdDev methods

In my opinion, those methods are a bit confusing, since they do more than returning the value and can be confused with traditional getters. I suggest that you pick a name that explain the action, example updateAndFetchSum.

toString method

I suggest that you add the java.lang.Override annotation when overriding the method, since I think it's easier to spot the inheritance when reading the code.

Unit Tests

In my opinion, the biggest issues to make the unit tests will be the uses of the static methods and variables in your code, since they will keep their states between tests. In your place, I would try to replace them with composition and a class for the utils, it will make the code more mockable and easier to test.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ I agree with NumberFormat. I seemed to have forgotten about how useful it is. I'll definitely fix that. Also, I'll try to make the Sample immutable and decouple the threshold and precision from the main class. Thanks for the great answer! I'll post a follow-up soon. \$\endgroup\$ – Hungry Blue Dev Feb 29 at 7:03
3
\$\begingroup\$

Threshold

Having static variables can cause issues with reusability and threading. I think having a default minimum threshold is reasonable. However, I'd consider removing the ability to change this default and make it possible to set a threshold when constructing the builder. This gives more control to the point that a sample is being constructed. It also makes it easier to use the class from different locations within the same application that may have different requirements around the number of observations needed.

Streams

Your buildSample creates a copy of observations as an array of Double, then copies it into an array of double in order to pass it to your sample class. Rather than doing that, you might want to consider using the stream api to simplify the code. Instead of:

Double[] rawOutput = observationList.toArray(new Double[count]);
double[] observations = new double[count];

for (int i = 0; i < count; i++) {
    observations[i] = rawOutput[i];
}

You end up with:

double[] observations = observationList.stream().mapToDouble(d->d).toArray();

Boxing

Your builder is based around a reference type Double, however the way that you add to the list is based around a native type double. Combine this with an addObservations that allows a collection of reference types and you're iterating through a list of Double converting it to double to process it for adding then back to Double again to put it in the list. This feels wrong to me, I'd code the addObservation around the type it needs for storage, or provide an overload so that you can call to reduce the amount of boxing/unboxing, particularly from within a loop.

Builder responsibility / Constructor responsibility

Your Sample class has a package private constructor, so my assumption is that your intention is to always use the SampleBuilder for construction. If this is the case then it's unclear which class is responsible for the sample size. The buildSample method throws an exception:

if (count < Sample.getThreshold()) {
    throw new IllegalStateException("The sample does not contain enough observations.");
}

In the Sample constructor throws a different exception:

if (observations.length < threshold) {
    throw new IllegalArgumentException(
        "The sample size is the less than the threshold: "
        + observations.length + " < " + threshold
    );
}

This seems like unnecessary confusion. Personally, I'd consider moving the constructor validation out of the Sample class, so that the complexity of validating the number of samples / creating a valid unit / name sits within the builder class and the Sample can assume it's being used correctly, since it's internal. If your intention is to reuse the Sample class with other things in the same package, then I'd remove all of the validation from the builder and just let the Sample constructor handle it. I would however try to avoid doing the same validation twice in different places, particularly with different outcomes.

Unit Tests

Generally you want to unit tests to be fast, isolated and repeatable. Your manipulation of the shared static threshold can break the isolation.

Your use of random to generate your samples can break the repeatability of your tests. Whilst sending in a random sample and getting out the right answer can be comforting, if you do find an issue, you're going to want to investigate and fix it. At the moment, you'll get an assertion error, telling you for example that two means don't match. Without information about the sample, it's going to be very difficult for you to repeat the test, you'll just know there's something wrong in an unknown scenario. So, if you do want to use random samples, make sure that if an error occurs you feedback what sample you were using.

A lot of your tests run 100 times, with different sample sizes. This is obviously going to be slower than running each test case a lower number of times. If I run an operation against sample sizes of 101 and 102, I'd expect them both to work, however I wouldn't usually write a unit test for both values, I'd focus on areas that I think might be relevant (for example, empty sample, below threshold, at threshold, just over threshold, well over threshold, possibly a really large sample). This means less wasted cycles and again, improved repeatability.

assertEquals(expected, actual)

You're using assert the wrong way round, which will be misleading if you encounter any errors, it should be the expected argument first.

Builder / no builder

Some of your tests use builders, some directly create a sample. If the intention is for the builder to always be used from client code, then it should be used for all of your unit tests, you shouldn't short-cut it and create a Sample class without the builder because the test becomes unrealistic, it's on a par with calling private methods. There's little point testing a class in a state that clients can't get it into.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ I'll definitely take your suggestions into consideration. All the points suggested are great, especially the Stream solution. I need to refactor now when there's relatively less work invested, rather than later. Thanks for such a great answer! I'll post a follow-up soon. \$\endgroup\$ – Hungry Blue Dev Feb 29 at 7:01
3
\$\begingroup\$

1) Persistant Data Structure

I think this could be a good use case for a Persistant Data Structure.

1.1) The Reason

The builder gives you some flexibility to observe at multiple locations

SampleBuilder builder = new SampleBuilder("Runtimes", "s");

// observe
for (...) { / * ... */ }

Sample runtimes = builder.buildSample();
System.out.println(runtimes);

// observe again
for (...) { / * ... */ }

Sample moreRuntimes = builder.buildSample();
System.out.println(moreRuntimes);

The down sight of the two inconspicuous System.out.println(...) is that you calculate everything multiple times. In the following sum and mean will calculate the first time:

Sample runtimes = builder.buildSample();
System.out.println(runtimes);

Now you calculate again sum and mean and can't reuse the previous calculation from the first calculation because you create with builder.buildSample() a new Sample instance:

Sample moreRuntimes = builder.buildSample();
System.out.println(moreRuntimes);

1.2) Possible Solution with a Persistant Data Structure

I created an executable example on repl.it, where a Sample can be Empty or NonEmpty. Please excuse that I ignored the threshold..

For an Empty Sample we know that the sum, mean and count would be 0. If we add a new observation to an Empty Sample the sum will be the observation, the count will be 1 and the mean is 0:

public Sample add(double observation) {
  return new NonEmptyBuilder().withIncrementedCount(0)
                              .withSum(observation)
                              .withMean(0)
                              .build();
}

Adding a new observation to a NonEmpty Sample we need to increment the count, add the previous sum with the new observation and calculate the mean:

public Sample add(double observation) {
  return new NonEmptyBuilder().withIncrementedCount(count)
                              .withSum(sum + observation)
                              .withMean(mean / count)
                              .build();
}

Since we calculate always the sum based on the previous sum we do not calculate observation multiple times:

// ...
System.out.println(sample.sum());

sample = sample.add(...);

// reuses previous sum and does not need to loop through all observations
System.out.println(sample.sum()); 

2) Separate the Summary from the Sample

Beside the fact that it violates the Open-Close- and the Single-Responsibility-Principle you limit the client to print a summary in a prescribed format to the console.

It would be nice to choose the output and the format of the output.

3) Sample has a low cohesion

This point is related to the previous.

In general a class should have a high cohesion.

When we look into Sample we can see the following methods:

private static String format(final double value) {
   return String.format("%." + precision + "f", value);
}

private static String format(final int value) {
   return String.valueOf(value);
}

private static boolean stringIsEmpty(final String text) {
   return text == null || text.isEmpty();
}

All are private because they don't belong to an API of a Sample and they are only used maximal at two spots which is a sign that they maybe don't belong into Sample.

4) No Independent Samples

The client cant have different instances of a Sample with different threshold and precision what limits the client in his/her possibilities.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ This is a neat idea to use Persistent Data Structures! I like the simple implementation that you provided too. I'll use this and even decouple the threshold and precision from the main class. Maybe delegate them to different classes. I'll post a follow-up soon! \$\endgroup\$ – Hungry Blue Dev Feb 29 at 7:07

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.