Ytdyklly

I'm doing the MOOC Java course (CTRL+F "distribution"):

The input of the program is a set of exam scores of a course. Each score is an integer. When -1 is entered, the program stops asking for further input.

After the scores have been read, the program prints the grade distribution and acceptance percentage of the course.

Grade distribution is formed as follows:

• Each exam score is mapped to a grade using the same formula as in exercise 18. If the score is not within the range 0-60 it is not taken into account.




• The number of grades are printed as stars, e.g. if there are 2 scores that correspond to grade 5, the line 5: ** is printed. If there are no scores that correspond to a particular grade, the printed line is 4:

All the grades besides zeros are accepted, so in the above 7 out of 8 participants were accepted. Acceptance percentage is calculated with the formula 100*accepted/allScores.

The formula in exercise 18 is:

<table >

      <tr>

        <th>Points</th>

        <th>Grade</th> 



      </tr>

      <tr>

        <td>0-29</td>

        <td>Failed</td> 



      </tr>

      <tr>

        <td>30-34</td>

        <td>1</td> 

      </tr>

       <tr>

        <td>35-39</td>

        <td>2</td> 

      </tr>

       <tr>

        <td>40-44</td>

        <td>3</td> 

      </tr>

       <tr>

        <td>45-49</td>

        <td>4</td> 

      </tr>

       <tr>

        <td>50-60</td>

        <td>5</td> 

      </tr>

    </table>

I wrote code that seems to work fine based on my tests, no compile errors, no run-time errors (except for cases where you enter a string instead of a number or a ridiculously large number).

I tested it using input from the table above, i.e I entered the numbers (0, 29, 30, 34, 35, 39, 40, 44, 45, 49, 50, 60), and there were two stars printed in each grade range, as expected. I also tested it with numbers in between the ranges, and numbers outside of [0, 60]. I found no logical errors.

I found the names I used too repetitive, GradeDistribution class, 3 arrays called gradeRanges, gradeDistributionList, gradeList.

Other problems: The logic of calculating gradeDistribution seems too nested. Should I have used switch-case? Should I have used static methods instead of a class? Should I have combined the methods calculateGradeDistribution and printGradeDistribution into one? Am I worrying too much about little things?

This is main:

import java.util.ArrayList;

import java.util.Scanner;



public class Main {

    public static void main(String args) {

        Scanner reader = new Scanner(System.in);

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

        int number = 0;

        System.out.println("Type exam scores, -1 to end");

        do{

            number = Integer.parseInt(reader.nextLine());

            if(number != -1){

                grades.add(number);

            }

        }while(number != -1);



        GradeDistribution syrianGradeDistribution = new GradeDistribution();

        syrianGradeDistribution.calculateGradeDistribution(grades);

        syrianGradeDistribution.printGradeDistribution();

        System.out.println(syrianGradeDistribution.acceptancePercentage());

    }

}

This is the GradeDistribution class:

import java.util.ArrayList;

import java.util.Collections;



public class GradeDistribution {

    private ArrayList<Integer> gradeRanges = new ArrayList<>();

    private ArrayList<Integer> gradeDistributionList = new ArrayList<>();



    public GradeDistribution(){

        Collections.addAll(gradeRanges, 0, 30, 35, 40, 45, 50, 61);

        Collections.addAll(gradeDistributionList, 0, 0, 0, 0, 0, 0);

    }



    public void calculateGradeDistribution(ArrayList<Integer> gradeList){

        for(int grade: gradeList){

            if(grade < 0 || grade > 60){ //invalid grades

                continue;

            }

            for(int i = 0; i < gradeRanges.size() -1 ; i++){

                 if(grade >= gradeRanges.get(i) && grade < gradeRanges.get(i + 1)){

                    gradeDistributionList.set(i, gradeDistributionList.get(i)+ 1);

                 }

            }

        }

    }



    public void printGradeDistribution(){

        for(int i = 0; i < gradeDistributionList.size(); i++){

            System.out.print(i + ": ");

            for(int j = 0; j < gradeDistributionList.get(i); j++){

                System.out.print("*");

            }

            System.out.println();

        }

    }



    public double acceptancePercentage(){

        int allScores = 0;

        for(int number: gradeDistributionList){

            allScores += number;

        }

        int acceptedScores = allScores - gradeDistributionList.get(0);

        double acceptancePercentage = 100.0 * acceptedScores / allScores;

        return acceptancePercentage;

    }

}

The code compiles without any errors on Windows 10, Java 11.0.2.

This is a very simple program, but yet, I still have many questions about the choices I made. How can I become confident in my choices and know what's acceptable and what's not?

I do not see any errors in the code. However, I do not like the choice of data structure in GradeDistribution. Not only are insertions slow -- taking time linear in the number of ranges -- but it is also unnecessarily involved. Java has a wealth of data structures in the standard library. Choosing the right one can make your code faster and cleaner. I recommend TreeMap, which has a floorEntry method that makes is easy to find the appropriate range for a given score.

More detailed critique:

• Use constants to hold magic numbers like the min/max scores and ranges.


• Add scores to the data structure one at a time instead of as a list.


• Shorten variables and method names.


• Use name getAcceptedPercent to make it clear this method is an accessor.


• Use Scanner.nextInt and Scanner.hasNextInt.


• Use an infinite loop and break to avoid checking number != -1 twice.


• Keep track of total and accepted scores as you go.

Here is my implementation, which addresses each issue:

import java.util.Scanner;



public class Main {

    public static void main(String args) {

        Scanner reader = new Scanner(System.in);

        GradeDistribution grades = new GradeDistribution();



        System.out.println("Type exam scores, -1 to end");

        while (reader.hasNextInt()) {

            int number = reader.nextInt();

            if (number == -1) {

                break;

            }

            grades.add(number);

        }



        grades.print();

        System.out.println(grades.getAcceptedPercent());

    }

}

import java.util.TreeMap;

import java.util.Map;



public class GradeDistribution {

    private static final int MIN = 0, MAX = 60;

    private static final int RANGE_STARTS = {0, 30, 35, 40, 45, 50};



    private TreeMap<Integer, Integer> rangeCount = new TreeMap<Integer,Integer>();

    private int totalScores = 0, acceptedScores = 0;



    public GradeDistribution(){

        for (int s : RANGE_STARTS) {

            rangeCount.put(s, 0);

        }

    }



    public void add(int grade) {

        if (grade < MIN || grade > MAX) {

            return;

        }



        Map.Entry<Integer,Integer> e = rangeCount.floorEntry(grade);

        rangeCount.put(e.getKey(), e.getValue() + 1);



        totalScores++;

        if (e.getKey() > MIN) {

            acceptedScores++;

        }

    }



    public void print(){

        int rangeNum = 1;

        for (int count : rangeCount.tailMap(MIN, false).values()) {

            System.out.printf("%d: ", rangeNum++);

            for (; count > 0; count--) {

                System.out.print('*');

            }

            System.out.println();

        }

    }



    public double getAcceptedPercent(){

        return (100.0 * acceptedScores) / totalScores;

    }

}

There is another further improvements you could consider, though I did not implement it here for the sake of brevity. You could use a mutable type for the map values. If, say the map was of type TreeMap<Integer,WrappedInteger> for a type WrappedInteger that supports an increment operation, we could write e.getValue().increment() instead of the put in the current implementation. This is potentially faster, as it avoids accessing the tree again.