Return to Revisions

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. So 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 requirments, 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. You'd then call a method to perform said calculation and either return the value, or have it set a field in your class that has a public getter. Here is basically what I'm saying in code

public class MarkCalculator {

    private final int[][] studentData;

    public MarkCalculator(int[][] studentData) {
        this.studentData = studentData;
    }
    
    //option one
    public int[] computeMarks() {
        //...
    }
    
    //option two
    private int[] marks;
    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 the data. If you pass in a MarkCalculator to whatever you are hoping to do with the data I believe it leaves less reason to change in the future. for example: your data type could change, the format of the data could change or any other number of reasons. The classes that depend on the data were built specifcally for this data, so if you pass in the class that generates this data I believe that you'll have less reasons to change. Now this isn't always the case and you'll want to weight the pro's and con's before you commit.

I caught in the code that you have "debug" code in. I have two things to say about this. Create a class that shows said data, or debug data and keep it out of your MarkCalculator. The second part is that I don't like using the console and visually inspecting your values. I would prefer to see some sort of unit test. Java has a few different testing frameworks and I'm not going to promote any particular one since they are similar in syntax and in reality they accomplish the same thing. They allow for you to write code and tests for that code to give you the desired feedback to know that your classes work as intended. So with that in mind here is a test I started to make and came accross 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/state that you'll ALWAYS have 6 modules. This would mean that MarkCalculator could be enhanced if you were to make it more dynammic. This test in place runs in less than 1 second and I can run it over and over and over again. When it fails my IDE gives me a big ol red X saying something failed. if it passes I get a nice green check mark. No guessing or reading required. 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. It is a integer and so does not need cleaning up, nor is it used outside of that method. There for it's declaration and value should be on the same line. After doing that my test still passes. Also you have a hardcoded the size of marks, yet it should be dynammic to match the number of modules. quick fix, and run my tests and it still works. int[] moduleMarks = new int[studentData.length]; 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. IMO pulling out this small bit of logic into a small method makes it easier to see how it can be rewritten (if possible). Indeed it can happen too. 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. Now unless I'm wrong (and you can proove otherwise) your new method would like 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'd have more, but I'll leave it for someone else.