Return to Answer

fixed grammer.

Source Link

edited Dec 23, 2015 at 21:25

3.1k
2
15
28

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 inIn 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 requirmentsrequirements, 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. How you get the calculation depends you could either return the value, or have itthe 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;
    }
    
    //option one
    public int[] computeMarks() {
        //...
    }
 }

#Option two

public class MarkCalculator {
    //optionprivate twofinal 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 thethis specific data. IfOn the classes that you want to use this data you would pass in aan instance of MarkCalculator to whatever you. This tells people who are hoping to do with the data I believe it leaves less reasongoing to change in the future. for example: your data type could change, the format ofuse the data could change or any other number of reasons. The classes that depend on the data were built specifcallyit was made specifically for this data, so if you pass in the class that generates this data I believe that you'll have less reasons to changewas 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, or debug data and keep it out of your MarkCalculator. The second part is that I don't2) other programmers might not like using the console and visually inspecting your values. I would preferto inspect data nor do they want to see some sortthe clutter of unitcommented out debug code. Using an automated test framework can fix this. Java has a few different testing frameworks and, but I'm not going to promote any particular one since they are similar in syntax and in reality they accomplish the same thing. They allowThese 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 youtests to write codeknow how to use the class and see what type and format the data needs to be in. With the tests for thatisolated from production code to give you don't clutter up the desired feedback to know that your classes work as intended. Soproduction code with "debug" code. With that in mind here is a test I started to make and came accrossacross a potential refactoring point/bug fix

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 dynammicdynamic. This test in place runsTests 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 Ithey can be run it over and over and over againrepetitively). When it fails my IDE gives meshows a big ol 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. No guessing or reading required and progress bar. NowNow 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 aIts an integer and so does not need cleaningany resources cleaned up, nor is it used outside of that method. There for it'sTherefor 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 hardcodedhard coded the size of marks, yet it should be dynammicdynamic to match the number of modules. quick fix, and run my tests andI changed it still works.to int[] moduleMarks = new int[studentData.length]; Movingand 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! theThe 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 pullingPulling out this small bit of logic into a small method also makes it easier to see how itthe logic 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 The refactored code that I mentioned specifically looks like this:

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

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

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

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

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;
    }
}

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

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:

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

Source Link

created Dec 22, 2015 at 4:16

Robert Snyder

3.1k
2
15
28

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.

@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);
}

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.