1
\$\begingroup\$

This is a simple Java program I made that takes in any number, and when it gets to 0, it returns the quantity of the numbers, the min, max, sum, and average of the numbers. Are there any ways how I can make the code more readable or improve the performance?

import java.util.Scanner;

public class Stats {
    public static void main(String[] args){
        Scanner scan = new Scanner(System.in);
        int size = 0;
        double max = 0;
        double min = 0;
        double sum = 0;
        double avg = 0;
        String num = null;
        do {
            num = scan.nextLine();
            try {
                double n = Double.parseDouble(num);
                if(!num.equals("0")){
                    if(size == 0){
                        max = n;
                        min = n;
                        sum = n;
                        avg = n;
                    } else {
                        max = Math.max(n, max);
                        min = Math.min(n, min);
                        sum += n;
                        avg = ((avg*size)+n)/(size+1);
                    }
                    size++;
                }
            } catch(NumberFormatException ex){
                System.out.println("Please input a number, try again.");
            }
        } while(!num.equals("0"));
        System.out.println("size: " + size +
                        "\nmin: " + min +
                        "\nmax: " + max +
                        "\nsum: " + sum +
                        "\navg: " + avg);
    }
}
\$\endgroup\$
1
  • \$\begingroup\$ That average calculation looks very inaccurate, given the nature of even double-precision floating point. \$\endgroup\$ Commented Jun 27, 2021 at 15:34

2 Answers 2

8
\$\begingroup\$

First of all, stop thinking about performance. This is not a tight loop, this is not a million numbers you crunch, the only concern you should have is readability and maintainability.

Now, your code does all at once in a single method. You do input, calculation and output, calculating as you go along. This is basically spaghetti.

You should aim to separate these concerns:

  • Write a method that does the input and returns a list of numbers
  • Write a method that does the calculation and returns some kind of statistics object, which contains your min/max/sum/avg
  • Write a method that takes this statistics object, and performs the output

As a template for further studies, something like this:

public class Statistics {
    private int min;
    private int max;
    private int sum;
    private int avg;

    ... construction, getters, setters
}

...

public static void main(String[] args) {
    List<Integer> numbers = readInput();
    Statistics statistics = computeStatistics(numbers);
    writeStatistics(statistics);
}

... add the methods referenced above
\$\endgroup\$
2
  • \$\begingroup\$ Good advice and answer. Except that I would have said Statistics statistics = new Statistics(numbers); \$\endgroup\$
    – gervais.b
    Commented Jun 28, 2021 at 7:34
  • 2
    \$\begingroup\$ @gervais.b I actually thought about both ways. Either make a real business class from the statistics containing all logic, or use it as a dumb struct. In the end, I think I also would put the business logic into the statistics class IRL, but for the sake of "learning the next step" I think leaving the class simple will be a better answer for the OP. \$\endgroup\$
    – mtj
    Commented Jun 28, 2021 at 7:42
2
\$\begingroup\$

A few comments regarding style.

    int size = 0;

In the Java standard libraries the name "size" refers to number of elements in a collection. The use here is misleading because you're not tracking the size of something. you're counting the number of values that has been entered, so a better name would be count.

    double avg = 0;

The average of the input values is not needed during input processing so defining and updating it in in the loop is unnecessary. It is something that you can generate at the output phase from sum and count. You don't need the avg variable at all. If you would need it, just spell average. Unnecessary abbreviations just make code less readable.

    avg = ((avg*size)+n)/(size+1);

Average is the sum of entries divided by number of entries. This is a much too complicated way to calculate the average and increases inaccuracy (floating point numbers are not precise so every unnecessary mathematical opration reduces accuracy).

    double max = 0;
    double min = 0;
    double sum = 0;

By using primitive types and initializing them to zero you handily avoid the need to handle the special case where the user does not enter anything. But it doesn't make the program correct. If the user does not enter values, the minimum and maximum values were not zero, nor was the average. So you produce incorrect results. You have to use Double types initialized to null and make a check for count being zero when producing output.

This looks like class room code so I suppose the requirement to use zero as an exit condition came from your teacher. In general it is not a good idea to use a value that could be interpreted as valid input as an exit condition. Right now you can input positive and negative values but not a zero and that is a bit weird. An empty line would have been a better choice in my opinion.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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