Student Grades Calculator

Question

I am seeking a review of a previous project I completed in my first term at university. The submission was graded 18/20 however the feedback was minimal and gave no suggestions regarding further improvements.

The specification prohibited the use of any non-primitive data types.

Concise problem specification:

Students take 6 modules in Stage 1. Each module has two components - Exam and Coursework and each module can have a different mark for each. The returned mark for a module is calculated as follows:

computed module mark = ((coursework mark * coursework weighting) + (examination mark * (100 - coursework weighting))) / 100

If the exam mark and coursework mark are greater than or equal to 35 then the returned mark is the computed module mark. However, if either (or both) the exam or coursework mark is less than 35 then the returned mark is the minimum(35, computed module mark).

All marks are rounded to the nearest whole number.

The student's performance on a given module can then be recorded as one of:

• Pass, if the module mark is at least 40
• Compensatable Fail, if the module mark is less than 40 but at least 35
• Fail, otherwise

Once all the module marks have been determined, a Stage average may be computed. This is found by averaging the returned module marks. The Stage result can then be computed as:

• Pass, if all modules are recorded as a Pass
• Pass By Compensation, if the Stage average is at least 40, no module is recorded as a Fail, and there are one or two modules totalling at most 40 credits recorded as a Compensatable Fail.
• Fail, otherwise

The submission is required to produce a clear formatted output showing the marks of each module in addition to the overall stage result. A graph should also be generated to reflect these results.

Sample output log:

Module 1 - Coursework Weighting: 50 - Exam Mark: 80 - Coursework Mark: 75
Module 2 - Coursework Weighting: 50 - Exam Mark: 35 - Coursework Mark: 60
Module 3 - Coursework Weighting: 50 - Exam Mark: 70 - Coursework Mark: 25
Module 4 - Coursework Weighting: 50 - Exam Mark: 70 - Coursework Mark: 40
Module 5 - Coursework Weighting: 50 - Exam Mark: 80 - Coursework Mark: 50
Module 6 - Coursework Weighting: 50 - Exam Mark: 15 - Coursework Mark: 70

Stage result: Fail

Module 1: 77 
Module 2: 47
Module 3: 35
Module 4: 55
Module 5: 65
Module 6: 35

Mark Calculator:

public class MarkCalculator {


    /***
     * 
     * Computes a students module marks based on the formula provided by the School Of Computer Science (SOCS). <p>
     * Method accepts a 2 dimensional array which takes the following structure: <p>
     * {{Coursework weighting, Coursework Mark, Exam Mark},..} <p>
     * Method returns a 1 dimensional array which takes the structure: <p>
     * {Module mark, Module mark,..}
     * 
     * @param studentData
     * @return moduleMarks
     */

    public static int[] computeMarks(int[][] studentData) {

        // Necessary variable declarations:
        // moduleMarks - Array of integers to store the module marks achieved by the student
        // computedModuleMark - Variable storing the computed module mark : Rounding to the nearest integer

        int[] moduleMarks = new int[6];
        int computedModuleMark = 0;

        // Iterate through and resolve data stored in the 2D array

        for (int x = 0; x < studentData.length; x++) {

            int weighting = studentData[x][0];
            int coursework = studentData[x][1];
            int exam = studentData[x][2];

            // Computing module mark based on formula provided (+0.5 to force rounding to nearest whole)

            computedModuleMark = (int) ((((coursework * weighting) + (exam * (100 - weighting))) + 0.5) / 100);

            // Check both exam and coursework meet requirements
            // Store the necessary mark in the moduleMarks array at the corresponding index

            if (exam >= 35 && coursework >= 35) {
                moduleMarks[x] = computedModuleMark;

            } else if (exam < 35 || coursework < 35) {
                moduleMarks[x] = Math.min(35, computedModuleMark);
            }
        }

        // DEBUG : DISPLAY ALL MODULE MARKS
        // for(int p: moduleMarks){ System.out.println(p); }

        return moduleMarks;

    }


    /***
     * 
     * Computes the students stage result and returns a statement based on their performance.
     * Method takes a 1 dimensional array of integers, representing a students module marks, and produces 
     * a stage result based on the criteria provided by the School Of Computer Science (SOCS). <p>
     * Students may achieve one of the following: Pass, Compensatable Pass or Fail. <p>
     * The method will return one of the above.
     * 
     * @param moduleMarks
     * @return "Pass" || "Compensatable Pass" || "Fail"
     */

    public static String computeResult(int[] moduleMarks) {

        // Initialise variables representing each attainable result

        int pass = 0;
        int compensatableFail = 0;
        int fail = 0;

        // Initialise variable (0) - Storing total marks achieved by way of compensatableFails

        int compensatableFailTotal = 0;

        // Determine status of each module result and increment the appropriate result

        for (int mark : moduleMarks) {

            if (mark >= 40) {
                pass += 1;

            } else if (mark < 40 && mark >= 35) {
                compensatableFail += 1;

                // Update total marks achieved by way of compensatable fail
                compensatableFailTotal += mark;

            } else {
                fail += 1;
            }

        }

        // DEBUG : DISPLAY MODULE RESULTS
        // System.out.println("Pass: " + pass);
        // System.out.println("Compensatable Fail: " + compensatableFail);
        // System.out.println("Fail: " + fail);

        // Compute stage average

        int stageTotal = 0;

        for (int mark : moduleMarks) {
            stageTotal += mark;
        }

        int stageAverage = stageTotal / moduleMarks.length;

        // DEBUG : DISPLAY STAGE AVERAGE
        // System.out.println("\nStage Average: " + stageAverage);

        // Determine and return stage result

        if (pass == 6) {
            return "Pass";

        } else if (stageAverage >= 40 && fail == 0 && compensatableFail <= 2 && compensatableFailTotal <= 40) {
            return "Compensatable Pass";

        } else {
            return "Fail";
        }

    }

}

Summary:

import java.util.Scanner;

public class Summary {

    public static void main(String[] args) {

        // DEBUG : TEST DATA 
        // int[][] data = {{50,20,20},{50,30,30},{50,40,40},{50,50,50},{50,60,60},{50,70,70}};


        // Populate array with user input
        int[][] data = inputData();

        // Produce 1 dimensional array of module marks 
        int[] moduleMarks = MarkCalculator.computeMarks(data);

        // Constructor for graph object 
        StudentChart graph = new StudentChart(data);

        // Produce graph and print summary
        graph.draw(moduleMarks);


    }

    /***
     * 
     * Method provides functionality to accept student data from the user. Method will only accept inputs 
     * in the form of integers.No parameters are required. The method will return a 2 dimensional 
     * array of integers taking the following structure: <p> 
     * {{Coursework weighting, Coursework Mark, Exam Mark},..}
     * 
     * @return
     */

    public static int[][] inputData() {

        // Initialise a new scanner object
        Scanner sc = new Scanner(System.in);

        // Initialise empty 2D array of length [6][3]
        // Array Structure : {{Coursework weighting, Coursework Mark, Exam Mark},..}

        int[][] moduleMarks = new int[6][3];

        // Populate each element of array with numerical input from user (INTEGER)

        for (int x = 0; x < 6; x++) {

            System.out.println("Module " + (x + 1));

            System.out.print("Coursework Weighting: ");
            moduleMarks[x][0] = sc.nextInt();

            System.out.print("Exam Mark: ");
            moduleMarks[x][1] = sc.nextInt();

            System.out.print("Coursework Mark: ");
            moduleMarks[x][2] = sc.nextInt();

            System.out.println();

        }

         // DEBUG : DISPLAY ALL ELEMENTS OF 2D ARRAY 
         // for(int[] x : moduleMarks){ for(int y : x){ System.out.println(y); }}


        // Close scanner and return array

        sc.close();
        return moduleMarks;

    }



}

An implementation of the Bar class was provided to us as a graphics library, this was used by another class I wrote to construct the following graph. (The graph did not require labelling and was simply constructed by manipulating shapes on a canvas)

I am looking for feedback regarding the design of my solution - Anything which could be improved in the future or would not be encouraged when programming in the industry.

I'm using Java version 1.8.0_60.

Answer 1

Industry standard rarely would restrict you to sending and receiving only primitive data. There are a few reasons for it, but mostly its so that the code can be read easier, and meaning can be associated with numbers. In your example instead of passing in a multi-dimensional array (which gives little in the way of meaning to numbers) you would instead pass in a collection of some class. It equates to the same in the end, but sure is easier to read. I know you said that it was part of the requirements, but I couldn't help but speak up to this. On to your code: I am not a big fan of having a class with only public static methods. There are reasons for it, but in general it's not good practice. I've been seeing more and more people starting to prefer constructor parameters for a class. The idea is that you give the class everything it requires to start work, then call a method to perform said calculation. How you get the calculation depends you could either return the value, or have the method set a field in your class that has a public getter. Here is basically what I'm saying in code

Option one

public class MarkCalculator {
    private final int[][] studentData;

    public MarkCalculator(int[][] studentData) {
        this.studentData = studentData;
    }

    public int[] computeMarks() {
        //...
    }
}

Option two

public class MarkCalculator {
    private final int[][] studentData;

    private int[] marks;

    public MarkCalculator(int[][] studentData) {
        this.studentData = studentData;
    }

    public void computerMarks(){
        //...
    }
    public int[] getMarks(){
        return marks;
    }
}

For this scenario I think I would prefer to return void, and have a public getter for marks. The reason I would prefer that is it would appear (based on the commented out code, and the graph that you show) that you want to do different things based on this specific data. On the classes that you want to use this data you would pass in an instance of MarkCalculator. This tells people who are going to use the data that it was made specifically for this the data that was computed for MarkCalculator. Now this isn't always the case and you'll want to weight the pro's and con's before you commit to one style or another.

I caught in the code that you have "debug" code in. I have two things to say about this: 1) Create a class that shows said data. 2) other programmers might not like using the console to inspect data nor do they want to see the clutter of commented out debug code. Using an automated test framework can fix this. Java has a few different testing frameworks, but I'm not going to promote any particular one since they are similar in syntax and in reality they accomplish the same thing. These tests are a way to address both points. The first point is covered because it is a class that, typically, has "test" in its name. Other programmers will want to look for tests to know how to use the class and see what type and format the data needs to be in. With the tests isolated from production code you don't clutter up the production code with "debug" code. With that in mind here is a test I started to make and came across a potential refactoring point/bug fix

@Test
public void testComputeMarks() throws Exception {
    int[][] data =
        {
            {50,80,75},
            {50,35,60},
            {50,70,25},
            {50,70,40},
            {50,80,50},
            {50,15,70}
        };
    MarkCalculator markCalculator = new MarkCalculator(data);
    int[] marks = markCalculator.computeMarks();

    assertEquals(marks[0], 77);
    assertEquals(marks[1], 47);
    assertEquals(marks[2], 35);
    assertEquals(marks[3], 55);
    assertEquals(marks[4], 65);
    assertEquals(marks[5], 35);
}

I noticed that when writing this you require that you'll ALWAYS have 6 modules. This would mean that MarkCalculator could be enhanced if you were to make it more dynamic. Tests are a nice way of informing you (very quickly) if the changes made to a class are breaking anything (I say very quickly because most tests run in less than 1 second and they can be run repetitively). When it fails my IDE shows a big red X and red progress bar saying something failed and a description of why it failed. Likewise if it passes I get a nice green check mark and progress bar. Now lets dig into your code. It takes less than a a second to run this test so I'll run it frequently.

I see that computedModuleMark is declared outside of where it is used. Its an integer and so does not need any resources cleaned up, nor is it used outside of that method. Therefor its declaration and value should be on the same line. After doing that I check my test and it still passes. Also you have a hard coded the size of marks, yet it should be dynamic to match the number of modules. I changed it to int[] moduleMarks = new int[studentData.length]; and run my tests and it still works. Moving down we see some nicely declared variables stating what index 0,1, and 2 are. I like that! The last part to this method, though, has some logic that could and should be moved out to a small private method. This way you can replace the comment used to describe this next section with a method name that does just what the comment says. Pulling out this small bit of logic into a small method also makes it easier to see how the logic can be rewritten (if possible). Often times when refactoring multiple conditionals it is worthwhile to reverse the conditional and see if it clears things up. I actually found that doing so removes the need both conditionals. The refactored code that I mentioned specifically looks like this:

public int[] computeMarks() {
    //...

        // Computing module mark based on formula provided (+0.5 to force rounding to nearest whole)
        int computedModuleMark = (int) ((((coursework * weighting) + (exam * (100 - weighting))) + 0.5) / 100);

        computedModuleMark = getAdjustedComputedModuleMark(coursework, exam, computedModuleMark);
        moduleMarks[x] = computedModuleMark;
    }

    return moduleMarks;
}

private int getAdjustedComputedModuleMark(int coursework, int exam, int computedModuleMark) {
    if (exam < 35 || coursework < 35) {
        return Math.min(35, computedModuleMark);
    }
    return computedModuleMark;
}

I have more to say, but I'll leave it for someone else.

Answer 2

Magic number 6

The number 6 is everywhere in the code. In most places it would be better to replace with a constant, for example private static final int MODULES_NUM. Or when you have an array whose size naturally carries that meaning, then use that.

Decompose to more, smaller methods

Since of the methods are a bit long, and can be decomposed to smaller pieces. For example this snippet could easily be in its own method:

    int stageTotal = 0;

    for (int mark : moduleMarks) {
        stageTotal += mark;
    }

Excessive comments

Comments like this are unnecessary:

    // Produce 1 dimensional array of module marks 
    int[] moduleMarks = MarkCalculator.computeMarks(data);

Review and remove comments that don't add value. In fact it's best when the code speaks for itself.

About programming in the industry...

I am looking for feedback regarding the design of my solution - Anything which could be improved in the future or would not be encouraged when programming in the industry.

As Robert also mentioned, prohibiting the use of objects defeats the purpose of an object oriented language like java. This assignment would have been better in a language like C. Using objects should be encouraged.

Answer 3

In Java, something like:

int wrong = 7/2; // This is 3, not 4 as you may expect.

is wrong. You noticed this, and added + 0.5 to get it to round as you'd expect. But this is an ugly solution that I'm not even sure works all of the time. I would instead suggest to just bite the bullet and use floating point numbers instead. That way there's no fancy tricks involved. Fancy tricks are evil. Dumb code that works well is good. And it's harder to write dumb code that anyone can understand.

Okay, so what is next?
Your calculator is kind of weird. Why have the methods be static? Why not just make the object, hand it the int[][] and let it do the rest for you?
Others have mentioned magic variables, and I agree that it's a bit silly. A way to make it at least a bit clearer what you're talking about is to use variables to refer to them. Compare:

float weight = [i][0]; // Why 0?
float weight = [i][WEIGHTING]; // Ah, so it's the index of the weighting!

This is just a small thing, but every little bit helps.

You can of course take three or so strategies.

• Compute the results every time
• Create them when you create the object and store them
• Create them the first time someone asks for them and then store them

Options one and two are the simplest ones, so unless you have a pressing reason to use #3, stick with one of those. As for which ones, at the level you're right now, flip a coin. :) I used #2 in my example, but really, I made both methods. compute[something] creates the answer, and get[something] just retrieves it.

Anyway, on to your if/else things.

Look at your if/else when you compute the result as a string. First you ask if mark >= 40. Then afterwards you ask if mark < 40 and if it's >= 35. But if it's not >= 40 then you already know that it must be less than 40, so there's no point in asking. You do this other places too. If the answer to exam >= 35 && coursework >= 35 is false, then we KNOW that either exam < 35 or coursework < 35 or both. So you don't have to test.

And there's a little bit more here, you should never have if/elseif and then nothing. Because the rest of us are going to get worried that we don't hit either, and you probably meant at least one of them to trigger. It's almost always better to do if/elseif/else because that way you know that one of them will be true.

The final problem I have with this is that it's a calculator that calculates marks. The obvious name is "Markulator". How could you not call it that?! :)

So, on to the command line interface you wrote. The only real problem I have with it is that you have used a bunch of magic variables. Again, naming them in Markulator and just referring to them helps demystifying it a bit. Making classes demystifies even more, but that was forbidden.

The final two things I'd like to point out is that you should remove the debug-statements, not comment them out when you turn in coursework or commit to source control, and that the standard variable for an index in an array is i, not x. i is universally understood as "an index in the array", but x means "some number". Oh, and you don't need three stars to make a javadoc, but two.

Finally, here's my suggestions of slightly better code. The isTest stuff is just me being lazy and is not recommended. In the real world, use automated unit testing instead.

Here's my suggestion for a better calculator:

/**
 * Calculates marks and stuff
 */
public class Markulator{
    public final static int NUMBER_OF_MODULES = 6;

    public final static int WEIGHTING = 0;
    public final static int COURSEWORK = 1;
    public final static int EXAM = 2;

    private final int[][] grades;
    private final int[] moduleMarks;
    private final String result;

    public Markulator(int[][] grades) {
    if(grades.length != NUMBER_OF_MODULES) {
        throw new IllegalArgumentException(String.format("Grades must be 6 numbers, was %d.", grades.length));
    }
    this.grades = grades;
    this.moduleMarks = computeModuleMarks();
    this.result = computeResult();
    }

    private int[] computeModuleMarks() {
    int[] moduleMarks = new int[NUMBER_OF_MODULES];

    for(int i = 0; i < grades.length; ++i) {
        // Look! It's almost like a map or a class now! ;_;
        float weight     = grades[i][WEIGHTING]; 
        float coursework = grades[i][COURSEWORK];
        float exam       = grades[i][EXAM]; 

        // Let's split the ugly formula into smaller easier chunks.
        float weightedCourse = coursework * weight;
        float weightedExam = exam * (100 - weight);
        float mark = (weightedCourse + weightedExam) / 100;

        // Then we just round it off. No need for fancy tricks.
        int roundOff = Math.round(mark);

        // Now we need to check if either is below 35
        // You only need one conditional though. I prefer this,
        // you may prefer the other. Matter of taste I think.
        if(exam < 35 || coursework < 35) {
        moduleMarks[i] = Math.min(35, roundOff);
        } else {
        moduleMarks[i] = roundOff;
        }
    }

    return moduleMarks;
    }

    public int[] getModuleMarks() {
    return moduleMarks;
    }


    public String getResult() {
    return result;
    }
    private String computeResult() {
    int pass = 0;
    int compensatableFail = 0;
    int fail = 0;
    int compensatableFailMarks = 0;
    int stageTotal = 0;

    for(int mark : getModuleMarks()) {
        stageTotal += mark;

        if(mark >= 40) {
        ++pass;
        } else if(mark >= 35) { // We already know it's less than 40
        ++compensatableFail;
        compensatableFailMarks += mark;
        } else {
        ++fail;
        }

    }

    double stageAverage = (double)stageTotal / (double)NUMBER_OF_MODULES;
    String out = "";
    if(pass == NUMBER_OF_MODULES) {
        out = "Pass";
    } else if(stageAverage >= 40        &&
          fail == 0                     &&
          compensatableFail <= 2        &&
          compensatableFailMarks <= 40) {
        out = "Compensatable Pass";
    } else {
        out =  "Fail";
    }

    return out;
    }
}

And here is the UI for it:

import java.util.Scanner;

/**
 * UI for the Markulator
 */
public class Cli {

    public static void main(String[] args){
    // This lets me type java cli test and then not have to enter anything
    boolean isTest = args.length > 0 && args[0].equalsIgnoreCase("TEST");

    System.out.println("Welcome to the Markulator!");
    int[][] data = inputData(isTest);

    Markulator mark = new Markulator(data);
    int[] moduleMarks = mark.getModuleMarks();

    //  StudentChart graph = new StudentChart(data);
    //  graph.draw(moduleMarks);

    generateFormattedOutput(mark);
    }

    private static void generateFormattedOutput(Markulator mark) {
    for(int i = 0; i < Markulator.NUMBER_OF_MODULES; ++i) {
        System.out.printf("Module %d: %d%n", i+1, mark.getModuleMarks()[i]);
    }
    System.out.printf("%nOverall stage result: %s%n", mark.getResult());
    }

    private static int[][] inputData(boolean isTest) {
    // This is just me being lazy.
    if(isTest) {

        System.out.println("Faking data input...");
        int[][] data = {{50,20,20},
                {50,30,30},
                {50,40,40},
                {50,50,50},
                {50,60,60},
                {50,70,70}};
        return data;
    }
    int[][] data = new int[6][3];
    Scanner keyboard = new Scanner(System.in);
    for(int i = 0; i < Markulator.NUMBER_OF_MODULES; ++i) {
        System.out.printf("Module %d:%n", i+1);
        System.out.print("Coursework weighting: ");
        data[i][Markulator.WEIGHTING] = Integer.parseInt(keyboard.nextLine().trim());
        System.out.print("Exam mark: ");
        data[i][Markulator.EXAM] = Integer.parseInt(keyboard.nextLine().trim());
        System.out.print("Coursework Mark: ");
        data[i][Markulator.COURSEWORK] = Integer.parseInt(keyboard.nextLine().trim());
        System.out.println();
    }
    keyboard.close();

    return data;
    }

}

Answer 4

I really would have to question the requirement to only use primitives... it would seem to be a rather arbitrary (and not especially useful) requirement even in an academic setting. The code would be a lot easier to write and test etc.

There are a relatively large number of Magic Numbers in your code. These can lead to problems later should the spec need to change, something which is guaranteed to happen in the real-world.

The array handling has magic numbers too. A good habit is to define an enum with values for the fields, though in this case you could use some public final int as constants to get around the primitives only requirement. This makes it clearer when you come back to the code in 3 years time (as would eventually happen in the real world). Take these two lines and decide which is clearer...

someArray[row][0] = sc.nextInt();
someArray[row][Marks.Fields.COURSEWORK_RATIO] = sc.nextInt();

...and yes, Java's all-caps convention isn't pretty.

You currently trust your user input and this is just asking for trouble in the real-world. You should as a minimum validate your input though knowing how much validation to use can require experience. Some basic bounds checking 100 >= x > 0 on the data entry certainly wouldn't go amiss.

Where you work with percentages (rather than them being raw data) I always find they're more intuitively represented if you store them as their actual value (where 50% is stored as 0.5 since it is 50/100.) This makes them far easier to use as working with them is always simple multiplication (and there's no repeated conversion between int to float and back).

Also, you're doing the rounding calculations manually and you should consider using Math.round() instead. It will compile to the same thing but it shows the intent of your code more clearly.

// original
int weighting = studentData[x][0];
int coursework = studentData[x][1];
int exam = studentData[x][2];
computedModuleMark = (int) ((((coursework * weighting) + (exam * (100 - weighting))) + 0.5) / 100);

// with weighting as a float
float weighting = studentData[x][Fields.WEIGHTING] / 100f;
int coursework = studentData[x][Fields.COURSEWORK];
int exam = studentData[x][Fields.EXAM];
computedModuleMark = Math.round(coursework * weighting + exam * (1.0 - weighting));

That should give you some more to work on.

PS: Did you spot the deliberate bug in the // original fragment? (answer below)

int coursework = studentData[x][1]; referred to the wrong array element.

This error would have been immediately obvious when using the more verbose notation which would have read int coursework = studentData[x][Fields.EXAM];