Grade Statistics

Question

I'm learning Java in a MOOC right now and this is my solution for exercise grade statistics.

This is the exercise:

In this exercise we create a program for printing statistics for points in course. The program receives points (integers from zero to one hundred) as input, based on which it prints statistics about grades. Reading of input stops when the user enters the number -1. Numbers that are not within the interval [0-100] should not be taken into account when calculating the statistics.

PART 1 Point averages

Write a program that reads integers representing course point totals from the user. Numbers between [0-100] are acceptable and the number -1 ends the reading of input. Other numbers are erroneous input, which should be ignored. When the user enters the number -1, the program should print the average of the point totals that were input.

PART 2 Point average for points giving a passing grade

Extend the program, such that it in addition to giving the point average of all totals also provides the point average for points giving a passing grade.

A passing grade is achieved by getting a minimum of 50 course points. You may assume that the user always provides at least one integer between [0-100]. If there are no numbers giving a passing grade, the program should print a line "-" where the average would be.

PART 3 Pass percentage

Extend the program from the previous part, such that it also print the pass percentage. The pass percentage is calculated using the formula 100 * passing / participants.

PART 4 Grade distribution

Extend the program, such that it also prints the grade distribution.Each point total is converted to a grade based on the above table. If a point total isn't within [0-100], it should be ignored.

The grade distribution is printed out as stars. E.g. if there is one point total giving the grade 5, then it should print the row 5: *. If there are no point totals giving a particular grade, then no stars should be printed for it. In the sample below this is true for e.g. the grade 4.

This is my solution:

Main.java:

import java.util.Scanner;

public class Main {

    public static void main(String[] args) {
        Scanner scanner = new Scanner(System.in);
    
        // Write your program here -- consider breaking the program into 
        // multiple classes.
        UserInterface ui = new UserInterface(scanner);
        ui.start();
    }
}

PointsStatistics.java:

import java.util.ArrayList;

public class PointsStatistics {

    private ArrayList<Integer> allpoints;
    private ArrayList<Integer> passingpoints;

    public PointsStatistics() {
        this.allpoints = new ArrayList<>();
        this.passingpoints = new ArrayList<>();
    }

    public void add(int points) {
        if (points > 0 && points >= 50 && points <= 100) {
            this.passingpoints.add(points);
        }
        if (points > 0 && points <= 100) {
            this.allpoints.add(points);
        }
    }

    public double pointsAverage() {
        double sum = 0.0;
        for (Integer points : this.allpoints) {
            sum += points;
        }
        return sum / this.allpoints.size();
    }

    public double possingPointsAverage() {
        double sum = 0.0;
        for (Integer points : this.passingpoints) {
            sum += points;
        }
        return sum / this.passingpoints.size();
    }

    public double passingPercentage() {
        double percentage = (this.passingpoints.size() * 1.0) / this.allpoints.size();
        return percentage * 100;
    }

    public void gradeDistribution() {
        ArrayList<Integer> grades = new ArrayList<>();
        for (Integer points : this.allpoints) {
            if (points < 50) {
                grades.add(0);
            } else if (points < 60) {
                grades.add(1);
            } else if (points < 70) {
                grades.add(2);
            } else if (points < 80) {
                grades.add(3);
            } else if (points < 90) {
                grades.add(4);
            } else if (points <= 100) {
                grades.add(5);
            }
        }

        for (int i = 5; i >= 0; i--) {
            System.out.print(i + ": ");            
            for(Integer grade : grades) {
                if (grade == i) {
                    System.out.print("*");
                }                
            }
            System.out.println("");            
        }        
    }
}

UserInterface.java

import java.util.Scanner;

public class UserInterface {
    private PointsStatistics points;
    private Scanner scanner;

    public UserInterface(Scanner scanner) {
        this.points = new PointsStatistics();
        this.scanner = scanner;
    }

    public void start() {
        System.out.println("Enter point totals, -1 stops: ");
        while(true) {
            int enterPoints = Integer.valueOf(scanner.nextLine());
        
            if( enterPoints == -1) {
                break;
            }
        
            this.points.add(enterPoints);            
        }
        System.out.println("Point average (all): " + this.points.pointsAverage());
        System.out.println("Points average (passing): " + this.points.possingPointsAverage());
        System.out.println("Pass percentage: " + this.points.passingPercentage());
        points.gradeDistribution();
    }
}

So what do you think? It works, but is it an optimal solution? Thank-you.

Answer 1

Package Name

Java code should be placed into a package with a name that avoids collisions with other developers. The best practice is to use the reverse-reading of some domain/account/email that you own, e.g.

package com.stackexchange.codereview.srxxx.grading;

Naming Conventions

Thumbs up for following the typical Java naming conventions.

Names

Take great care when introducing variable names, to make them as self-explanatory as possible.

One improvement: In passingPercentage(), you have a variable named percentage, but it's not in the 0..100 range you'd associate with that word, instead its between 0 and 1. So, I'd call it a ratio instead of a percentage.

This might sound like nitpicking, but believe me, it's the most important skill when working either in a team, or alone on a long-lived software project.

Separate Computation and User Interface

You mostly follow the separation of concerns between computation (class PointsStatistics) und user interface (class UserInterface).

Only for gradeDistribution(), you do System.out.print() inside PointsStatistics. You should change the method to return the list of grades (grades variable) and do the printing loop from the UserInterface class.

Document your Classes and Methods

Write documentation comments (Javadoc-style) for public classes and methods, documenting what their task is (not how they fulfill it).

For good Javadoc examples, have a look at the Java library classes, and you surely use the HTML-generated documentation results on web pages like the Java8 Platform Spec. In 20+ years of Java development, I rarely ever had to look into the Java source code, the Javadocs explained all the pre-existing Java classes and methods well enough.

Declare Variables with Most Generic Types

You should change

ArrayList<Integer> grades = new ArrayList<>();

to become

List<Integer> grades = new ArrayList<>();

The general rule is to declare a variable with the most generic type (or interface) that offers the methods and behaviour you need, and initialize it with the concrete class you (currently) want to use.

Why? Either believe me that it really is best practice, or read along the following lengthy explanation.

All of your code will work with any type of List, maybe a LinkedList or an array wrapped as a List (using Arrays.asList()). Maybe, later you find a super-cool new List implementation, you want to exchange the ArrayList with that class. Then you just exchange the initialization part to now read

List<Integer> grades = new SuperCoolList<>();

and everything works the same, just way cooler.

You may ask, why not do it for both the initialization and the declaration as well:

SuperCoolList<Integer> grades = new SuperCoolList<>();

The risk is that with the initial declaration ArrayList<Integer> grades nothing stopped you from calling e.g. grades.trimToSize(), which is not part of the List interface but specific to ArrayList. There are two possible situations:

• Probably, SuperCoolList will not have such a method, resulting in a compile error. You see the error, invest some time to invent some replacement for the method call, or find that you can't use the SuperCoolList and return to ArrayList.
• Much worse, the SuperCoolList might have such a method, but doing something different. Then the program will misbehave without you getting any hint by the compiler.

With

List<Integer> grades = new ArrayList<>();

this will not happen. Every method you call on the grades variable will be present in every kind of List implementation, and behave the same. You can't accidentally use the grades.trimToSize() method as this isn't part of the List interface. This way, the freedom to later change to a different List implementation is guaranteed by the compiler.

Answer 2

Bug

Your "Part 2" needs work. Specifically, the requirement is

If there are no numbers giving a passing grade, the program should print a line "-" where the average would be."

Your program crashes if no passing grade is given.

Redundant Tests

You are testing points > 0 and points <= 100 twice:

        if (points > 0 && points >= 50 && points <= 100) {
            this.passingpoints.add(points);
        }
        if (points > 0 && points <= 100) {
            this.allpoints.add(points);
        }

You should reorganize this to test the valid range once.

        if (points > 0 && points <= 100) {
            if (points >= 50) {
                this.passingpoints.add(points);
            }
            this.allpoints.add(points);
        }

Bug

The program receives points (integers from zero to one hundred) as input, based on which it prints statistics about grades.

Your points > 0 tests result in not including any zero-score points in the statistics. You probably should use points >= 0.

Spelling

possingPointsAverage() should probably be passingPointsAverage()

Type conversions

int to double

The * 1.0 in the following is just to ensure the division is not performed as integer division.

        double percentage = (this.passingpoints.size() * 1.0) / this.allpoints.size();

Barring compiler heroics, the passing point size must first be promoted to a double before the double * double multiplication can be performed:

        double percentage = ((double)this.passingpoints.size() * 1.0) / this.allpoints.size();
                             ^^^^^^^^

At this point, the multiplication by 1.0 is just busy-work which should be optimized away.

        double percentage = (double)this.passingpoints.size() / this.allpoints.size();

Any compiler worth its salt will probably do this optimization for you, but there are limits. And in this case, the awkward * 1.0 can be eliminated without inserting the cast by simply combining the operation with the next line, which multiplied the result by 100 to convert the ratio to a percentage.

        double percentage = 100.0 * this.passingpoints.size() / this.allpoints.size();
        return percentage;

Unboxing

        for (Integer points : this.allpoints) {
            if (points < 50) {
                grades.add(0);
            } else if (points < 60) {
                grades.add(1);
            } else if (points < 70) {
                grades.add(2);
            } else if (points < 80) {
                grades.add(3);
            } else if (points < 90) {
                grades.add(4);
            } else if (points <= 100) {
                grades.add(5);
            }
        }

In this loop points is an Integer, which is a "boxed primitive". To use it, the value must be unboxed to an int, by implicitly calling points.intValue() where it is used in integer context. I see 6 places where this happens: points < 50, points < 60, points < 70, points < 80, points < 90, points <= 100.

Again, any compiler worth its salt should optimize that for you, but it would be simpler just to unbox the result to an int once ... in the for statement:

        for (int points : this.allpoints) {
             ^^^

Is it optimal?

No.

        for (int i = 5; i >= 0; i--) {
            System.out.print(i + ": ");            
            for(Integer grade : grades) {
                if (grade == i) {
                    System.out.print("*");

If you have one hundred points scores, you will create a duplicate grades list which will have one hundred entries, each with either a 0, 1, 2, 3, 4, or 5. You then loop through the list of one hundred entries 6 times, once for each grade level. That executes the if (grade == i) statement six hundred times!

Instead, if you used something more like:

    public void gradeDistribution() {
        int grades[] = new int[] {0, 0, 0, 0, 0, 0};
        for (int points : this.allpoints)
            if (points < 50)
                grades[0]++;
            else if (points < 60)
                grades[1]++;
            else if (points < 70)
                grades[2]++;
            else if (points < 80)
                grades[3]++;
            else if (points < 90)
                grades[4]++;
            else if (points <= 100)
                grades[5]++;

        ...

Now, instead of a hundred grade values, you have only 6 values representing the count of each grade value. This should be easier to work with.

This

You almost never need to use this. inside methods. It just adds verbosity, and creates confusion when you accidentally forget to use it.

        System.out.println("Point average (all): " + this.points.pointsAverage());
        System.out.println("Points average (passing): " + this.points.possingPointsAverage());
        System.out.println("Pass percentage: " + this.points.passingPercentage());
        points.gradeDistribution();

What is the difference between this.points in the first three lines, and points in the last??? There isn't any, yet it looks different! The reader is left wondering "Is it intentional? Is there a reason for the difference I'm missing???" They have to look back and check if there is some variable or parameter shadowing the member name.

(There are times where you might need to use this., such as in a constructor or a setter function, you might pass in a parameter with the same name as a member, and have to use a statement like this.name = name;. In general, it is safer to use a slightly different name, such as adding an underscore to the parameter name and writing name = _name;)

Names

Avoid naming variables with jammed together names like allpoints and passingpoints. They get hard to read. Use underscores (all_points) or bumpy words (allPoints) to give the reader a chance because figuringoutwhereonewordendsandthenextbeginscanbehard.

Exception handling

If the user enters a grade of 49.5 or end, the program will crash with a NumberFormatException. Use try ... catch where user input occurs to prevent accidental/unnecessary crashes.