0
\$\begingroup\$

I'm seeking a review of correct design practices.

import java.math.BigDecimal;

public class Statistician {

    //instance variables
    private int length;
    private double sum;
    private double last;
    private double max;
    private double min;

    //choosing to use public b/c they are constants
    public static final BigDecimal doubleMax = BigDecimal.valueOf(Double.MAX_VALUE);
    //using -Double.Max_Value b/c Double.Min_Value will return the lowest value greater than zero. 
    //Since Double values are symmetrical about zero we can use the opposite of Double.Max_Value to get an accurate bound for the lower range.
    public static final BigDecimal doubleMin = BigDecimal.valueOf(-Double.MAX_VALUE);

    /**
     * Constructor Method; Initialize a new Statistician.
     *
     * @param - none
     * <dt><b>Postcondition:</b><dd>
     * This Statistician is newly initialized and has not yet been given any
     * numbers.
     *
     */
    public Statistician() {
        length = 0;
        sum = 0;
    }

    /**
     * Give a new number to this Statistician.
     *
     * @param <CODE>number</CODE> the new number that is being given the this
     * Statistician
     * <dt><b>Postcondition:</b><dd>
     * The specified number has been given to this Statistician and it will be
     * included in any subsequent statistics.
     *
     */
    public void nextNumber(double number) {

        length++;
        sum += number;
        last = number;

        if (length == 1) {
            max = number;
            min = number;

        } 

        else {

            if (number > max) {

                max = number;
            }
        }

            if (number < min) {

                min = number;
        }
    }

    /**
     * Determine how many numbers have been given to this Statistician.
     *
     * @param - none
     * @return the count of how many numbers have been given to this
     * Statistician
     * <dt><b>Note:</b><dd>
     * Giving a Statistician more than <CODE>Integer.MAX_VALUE</CODE> numbers,
     * will cause failure with an arithmetic overflow.
     *
     */
    public int getLength() {
        // The student's code will replace this return statement: and so it came to pass
        if (length > Integer.MAX_VALUE) {

                throw new RuntimeException("Arthmetic Overflow Occured");
            }

        else {

            return length;
        }
    }

    /**
     * Determine the largest number that has been given to this Statistician.
     *
     * @param - none
     * @return the largest number that has been given to this Statistician
     * <dt><b>Note:</b><dd>
     * If <CODE>length()</CODE> is zero, then the answer from this method is
     * <CODE>Double.NaN</CODE>.
     *
     */
    public double getMaximum() {
        // The student's code will replace this return statement: indeed
        if (length == 0){

            return Double.NaN;
        }

        else {

            return max;
        }
    }

    /**
     * Determine the arithmetic average of all the numbers that have been given
     * to this Statistician.
     *
     * @param - none
     * @return the arithmetic mean of all the number that have been given to
     * this Statistician
     * <dt><b>Note:</b><dd>
     * If this Statistician has been given more than
     * <CODE>Integer.MAX_VALUE</CODE> numbers, then this method fails because of
     * arithmetic overflow. If <CODE>length()</CODE> is zero, then the answer
     * from this method is <CODE>Double.NaN</CODE>. If <CODE>sum()</CODE>
     * exceeds the bounds of double numbers, then the answer from this method
     * may be <CODE>Double.POSITIVE_INFINITY</CODE> or
     * <CODE>Double.NEGATIVE_INFINITY</CODE>.
     *
     */
    public double getMean() {
        // The student's code will replace this return statement: And on the 7th day it was so
        if (length == 0) {
            return Double.NaN;
        } 

        else {

            if (doubleMin.compareTo(new BigDecimal(sum)) <= 0) {

                return Double.NEGATIVE_INFINITY;
            }

            if (doubleMax.compareTo(new BigDecimal(sum)) >= 0) {

                return Double.POSITIVE_INFINITY;
            }

            if (length > Integer.MAX_VALUE) {

                throw new RuntimeException("Arthmetic Overflow Occured");
            }

        }

        return sum / length;

    }


    /**
     * Determine the sum of all the numbers that have been given to this
     * Statistician.
     *
     * @param - none
     * @return the sum of all the number that have been given to this
     * Statistician
     * <dt><b>Note:</b><dd>
     * If the sum exceeds the bounds of double numbers, then the answer from
     * this method may be <CODE>Double.POSITIVE_INFINITY</CODE> or
     * <CODE>Double.NEGATIVE_INFINITY</CODE>.
     *
     */
    public double getSum() {
        // The student's code will replace this return statement: the prodigal sum
         if (doubleMin.compareTo(new BigDecimal(sum)) <= 0) {

                return Double.NEGATIVE_INFINITY;
            }

            if (doubleMax.compareTo(new BigDecimal(sum)) >= 0) {

                return Double.POSITIVE_INFINITY;
            }

            else {

                return sum;
            }
    }

    /**
     * Get the last number in the sequence of numbers Statistician.
     *
     * @param - none
     * @return the last number that was placed in the sequence
     *
     */
    public double getLastNum() {
        // The student's code will replace this return statement: last but not least
        return last;
    }
}
\$\endgroup\$
  • 4
    \$\begingroup\$ Please describe, in English, next to your code, what it does and what it's for. \$\endgroup\$ – Pimgd Sep 18 '14 at 8:26
  • \$\begingroup\$ Ok it's pretty heavily commented already, but, I will make it more explicit. To be honest, it's pretty straightforward. \$\endgroup\$ – Michael James Sep 18 '14 at 21:48
2
\$\begingroup\$

The code is a good first attempt at translating the specs, but it is open to refinement.

Consider the edge case on line 80: length > Integer.MAX_VALUE can never be true because length is of type int; instead, it will overflow to Integer.MIN_VALUE.

To solve this the easy way, have a look at java.math.BigDecimal, especially its intValueExact() method:

Converts this BigDecimal to an int, checking for lost information. If this BigDecimal has a nonzero fractional part or is out of the possible range for an int result then an ArithmeticException is thrown.

This looks exactly like what we need. And doubleValue() is looking very friendly as well:

Converts this BigDecimal to a double. This conversion is similar to the narrowing primitive conversion from double to float as defined in section 5.1.3 of The Java™ Language Specification: if this BigDecimal has too great a magnitude represent as a double, it will be converted to Double.NEGATIVE_INFINITY or Double.POSITIVE_INFINITY as appropriate. Note that even when the return value is finite, this conversion can lose information about the precision of the BigDecimal value.

Using BigDecimal essentially guarantees we'll follow the specs laid out here. That makes the code a lot easier on us (stripped the comments for brevity):

import java.math.BigDecimal;

public class Statistician {
    private BigDecimal length;
    private BigDecimal sum;
    private double last;
    private double max;

    public Statistician() {
        length = BigDecimal.ZERO;
        sum = BigDecimal.ZERO;
        max = last = Double.NaN;
    }

    public void nextNumber(double number) {
        last = number;

        if ( max == Double.NaN || max < number ) {
            max = number;
        }

        // BigDecimal is immutable; don't forget to assign the result!
        length = length.add(BigDecimal.ONE);
        sum = sum.add( BigDecimal.valueOf(number) );
    }

    public int getLength() {
        return length.intValueExact();
    }

    public double getMaximum() {
        return max;
    }

    public double getMean() {
        final int length = getLength();
        return length == 0 ? Double.NaN : getSum() / length;
    }

    public double getSum() {
        return sum.doubleValue();
    }

    public double getLastNum() {
        return last;
    }
}

While personally I cringe at the sight of using a decimal type to represent length, it seems the least-effort approach here. The alternative is checking in nextNumber whether length has overflown, and then set a flag to raise ArithmeticExceptions as required by the specs.

\$\endgroup\$
1
\$\begingroup\$

Aside from the use of BigDecimals functions mentioned in JvR's answer, I see one other glaring issue with your code.

Your indentation is all over the place.

/**
 * Give a new number to this Statistician.
 *
 * @param <CODE>number</CODE> the new number that is being given the this
 * Statistician
 * <dt><b>Postcondition:</b><dd>
 * The specified number has been given to this Statistician and it will be
 * included in any subsequent statistics.
 *
 */
public void nextNumber(double number) {

    length++;
    sum += number;
    last = number;

    if (length == 1) {
        max = number;
        min = number;

    } 

    else {

        if (number > max) {

            max = number;
        }
    }

        if (number < min) {

            min = number;
    }
}

I strongly suggest you find and use the auto-indent functions of your IDE.

As a example, here's how the code should look:

/**
 * Give a new number to this Statistician.
 *
 * @param <CODE>number</CODE> the new number that is being given the this
 * Statistician
 * <dt><b>Postcondition:</b><dd>
 * The specified number has been given to this Statistician and it will be
 * included in any subsequent statistics.
 *
 */
public void nextNumber(double number) {

    length++;
    sum += number;
    last = number;

    if (length == 1) {
        max = number;
        min = number;

    } else {
        if (number > max) {
            max = number;
        }
    }
    if (number < min) {
         min = number;
    }
}

Proper indentation makes it easier to find optimizations too. You could convert

    else {
        if (number > max) {
            max = number;
        }
    }

into

    else if (number > max) {
        max = number;
    }

without any risk. This makes the code even shorter, without reducing meaning. It effectively becomes easier to understand.

\$\endgroup\$

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.