Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I'm writing a program which takes individual int inputs from the user and then outputs some details on the numbers (average, min, max and how many). Input.readInt() is a method I made which simply reads an integer from the command line and returns it.

There are a few things I'd like to improve - for the average the float does not seem to work, it only holds prints out value to nearest integer. I'd also like to format this to always display 2 decimal places. Is there any way to dynamically assign the size of the array? So I could enter more than 10 values?

I'm looking for advice on more efficient methods (both speed and code length), and any other general advice which could help me learn.

public class NumberStats {

    public static void main(String[] args) {
        int[] input = new int[10];
        int i;
        int total = 0;
        int min = 0;
        int max = 0;
        double average = 0;
        int count = 0;

        while (true) { // READ INPUTS
            System.out.print("Please enter a number: ");
            input[count] = Input.readInt();

            if (input[count] == 0) {
                break;
            } else if (input[count] < 0) {
                System.err.println("Error: " + input[count]
                        + " is not valid, exiting.\n");
                break;
            } else if (input[count] > 100) {
                System.out
                        .println("Warning: "
                                + input[count]
                                + " is not valid and will be excluded from final statistics.");
            }

            if (input[count] <= 100) { // IF INPUT VALID
                if (count == 0) {
                    min = input[count];
                    max = input[count];
                } else if (input[count] < min) {
                    min = input[count];
                } else if (input[count] > max) {
                    max = input[count];
                }
                count++;
                System.out.println("#" + count + ": " + input[count - 1]);
            }

        }
        if (count != 0) {
            for (i = 0; i < input.length; i++) { // CALCULATE SUM
                total += input[i];
            }
            average = total / count; // CALCULATE AVERAGE

            System.out.println("Total: " + total);
            System.out.println("Count: " + count);
            System.out.println("Min: " + min);
            System.out.println("Max: " + max);
            System.out.println("Average: " + average);
        } else {
            System.out.println("No valid input");
        }
    }
}
share|improve this question

2 Answers 2

up vote 4 down vote accepted

Please tell me this is for your personal benefit and that you aren't asking me to do your homework!

average = total / count;

total and count are both integers, so / performs integer division. The result is then converted to a double. This should fix your problem:

average = ((double) total) / ((double) count);

Is there any way to dynamically assign the size of the array? so i could if i wish enter more than 10 values?

Use a java.util.ArrayList:

// old:
// int[] input = new int[10];
// new:
List<Integer> input = new ArrayList<Integer>();

I'd also like to format this to always display 2 decimal places

Look at java.text.DecimalFormat.

General Advice

You do an awful lot of typing and a lot of dereferencing (and typing!). Declare a local inVal parameter:

int inVal = 0;
...

inVal = Input.readInt();
if (inVal < 0) {
    ...
}
// Finally, after all your checks and calculations, add inVal to your array (or List):
input[count] = inVal;

The point of most loop syntax is to make the exit conditions for the loop obvious. Instead of while (true) I'd like to see something like:

do {
    ...
} while (inVal >= 0)

I like that you calculate your max and min in the input loop. You could count your total in that loop as well instead of taking a second loop through your data. Hmm... If you do that, you don't need to use an array or a List, just inVal.

You have "magic numbers" 0 and 100. Years from now when three other programmers have worked on this code, they may add a percentage calculation as x / 100 so that when you decide to increase your number of inputs to 200, you search-and-replace 100 with 200 you have broken your percentage calculation. You should factor these out of your code:

public final int MAX_NUM_INPUTS = 100;
public final int INPUT_TOO_LOW_VALUE = 0;

This clarifies what you are comparing in a way that Magic Numbers could not:

if (count > MAX_NUM_INPUTS) ...

if (inVal <= INPUT_TOO_LOW_VALUE) ...

Hopefully no-one would ever calculate percent as x / MAX_NUM_INPUTS. Now you can change these limits in one place without doing any search-and-replace.

UPDATE:

System.out.println...

Yeah, Java is funny about string concatenation. If you have a non-final variable as part of a String, it can cause a bunch of nasty overhead. For a little program like yours, it really doesn't matter what you do. If it were going to print out a String in a loop, then I'd suggest the following change. Note that System.err.println() uses an OS-specific line separator - not necessarily \n.

// This is fine for your program..
System.err.println("Error: " + input[count]
                        + " is not valid, exiting.\n");

// Option 1: Use multiple print stmts to avoid String concatenation:
System.err.print("Error: ");
System.err.print(inVal);
System.err.println(" is not valid; exiting.");
System.err.println();

// Option 2: Use a StringBuilder (also uses "method chaining" idiom):
System.err.println(new StringBuilder().append("Error: ")
                                      .append(inVal)
                                      .append(" is not valid; exiting.")
                                      .toString());
System.err.println();

This really isn't important in your specific example because it's so small. In a bigger program or a production situation String concatenation can be a real performance problem. People often make mistakes in logging code that really slows a system down. I think later versions of Java have tamed this issue. I know IntelliJ IDEA reports various times when it's better to use the + signs vs. a StringBuilder. I've checked the compiled code a few times when I thought it was wrong, and it turned out to be right, so now I just trust it to tell me.

share|improve this answer
    
Haha no this is a personal learning exercise i'm doing. thanks for the detailed advice and explanations, it helps alot! –  BradStevenson Feb 6 '13 at 19:26
    
Is it more acceptable to use system.out.println() repeatedly for the final out put or should I be using system.out.print and then format the string? –  BradStevenson Feb 6 '13 at 19:37

Glen Peterson's advice is good, but there's something I'd like to add:

You're programming in Java, an object-oriented language. When first learning to program, it's often hard to imagine the practical value of using objects. Therefore, as a beginner one often ends up with a style similar to yours: all code is in a single class, and indeed within a single method in that class.

This style is called monolithic and although it has the benefit of being very concise, it leads to unmaintainable, hard-to-debug and unreadable code. As the requirements evolve in complexity, these problems become worse.

To solve them, first analyse the responsibilities of your code:

  • Input/Output
  • Validation
  • Tracking statistics

These concerns should be handled by collaborating objects. For instance, to track statistics, you could create a StatisticsTracker:

public class StatisticsTracker {
    private int count;
    private int total;
    private int minimum;
    private int maximum;

    public void addNumber(int number) {
        count++;
        total += number;
        adjustMinimumAndMaximum(number);
    }

    private void adjustMinimumAndMaximum(int number) {
        if (containsSingleNumber()) {
            minimum = number;
            maximum = number;
        } else if (number < minimum) {
            minimum = number;
        } else if (number > maximum) {
            maximum = number;
        }
    }

    private boolean containsSingleNumber() {
        return count == 1;
    }

    public int getCount() {
        return count;
    }

    public int getTotal() {
        return total;
    }

    public int getMinimum() {
        return minimum;
    }

    public int getMaximum() {
        return maximum;
    }

    public double getAverage() {
        return (double) total / count;
    }
}

Notice that you don't need to hang on to an array or list of numbers; you can track the statistics as you add numbers. Please also note that all this code knows nothing about validation or input/output: all it cares about is tracking statistics.

Next, we'll need a collaborator that validates our input. Because I'm assuming your validation requirements are likely to change, it makes sense to define an interface (T means there can be validators for any type of data, such as strings, integers or floating-point numbers):

public interface Validator<T> {
    public ValidationResult validate(T input);
}

Our validator needs to check if the input is between two bounds, so let's implement the interface:

public class IntegerBoundsValidator implements Validator<Integer> {
    private int lower;
    private int upper;

    public IntegerBoundsValidator(int lowerBound, int upperBound) {
        lower = lowerBound;
        upper = upperBound;
    }

    @Override
    public ValidationResult validate(Integer input) {
        if (input < lower) {
            return ValidationResult.invalid("input must be greater than " + lower);
        }
        if (input > upper) {
            return ValidationResult.invalid("input must be smaller than " + upper);
        }
        return ValidationResult.valid();
    }
}

You'll notice the use of a class called ValidationResult. This is a Value Type. It has no significant behaviour; it just contains data and some static convenience methods to create instances of it, as seen below. Some OOP purists disagree with my use of public final fields; feel free to make them private and add getters if you so desire.

public class ValidationResult {
    public final boolean isValid;
    public final String message;

    private static final ValidationResult VALID = new ValidationResult(true, "");

    public static ValidationResult valid() {
        return VALID;
    }

    public static ValidationResult invalid(String message) {
        return new ValidationResult(false, message);
    }

    private ValidationResult(boolean isValid, String message) {
        this.isValid = isValid;
        this.message = message;
    }
}

To connect all these parts together, we'll use a class called Main - for the lack of a better name.

public class Main {
    private static final Scanner input = new Scanner(System.in);
    private static final Validator<Integer> validator = new IntegerBoundsValidator(0, 100);
    private static final StatisticsTracker statistics = new StatisticsTracker();

    public static void main(String[] args) {
        while (input.hasNextInt()) {
            int next = input.nextInt();
            handleInput(next);
        }
        printStatistics();
    }

    public static void handleInput(int input) {
        ValidationResult result = validator.validate(input);
        if (result.isValid) {
            statistics.addNumber(input);
        } else {
            printLine(result.message);
        }
    }

    private static void printStatistics() {
        if (statistics.getCount() == 0){
            printLine("No valid input.");
            return;
        }
        printLine("Total: %d", statistics.getTotal());
        printLine("Count: %d", statistics.getCount());
        printLine("Min: %d", statistics.getMinimum());
        printLine("Max: %d", statistics.getMaximum());
        printLine("Average: %.2f", statistics.getAverage());
    }

    private static void printLine(String format, Object... args) {
        System.out.println(String.format(format, args));
    }
}

Conclusion

Why go to all this effort of refactoring? Well, the benefits are apparent:

  • Each class now fulfils a clearly-defined responsibility. Changes in one of those classes will not affect the other responsibilities.
  • Each method within these classes is short and has a readable name. These methods can and should be tested separately with unit tests, which is close to impossible with monolithic code.
  • The system is extensible: It's easy to change your validation requirements by simply plugging in a different Validator instance. You could create a CompositeValidator that combines several validators into a single validator chain.

Of course, you may disagree with some of the implementation details — but this review isn't really about those. It's more of a demonstration of which questions you should be asking in order to end up with clean code.

Good luck with your learning, and thanks for sharing your code.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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