School grading string calculator

Question

For my training, i've been given an assignment to build a simple, extendable 'grade' String calculator by a given grade.

For now, I had to work by this table:

enter image description here

When the grade was higher then 100, I had to return the String "You must have been cheating".

What I currently came up with is:

public String gradeBy(int grade) {
 TreeMap<Boolean, Grade> grades = new TreeMap<>();
 grades.put(grade <= 50, Grade.F);
 grades.put(grade >= 51 && grade <= 60, Grade.D);
 grades.put(grade >= 61 && grade <= 70, Grade.C);
 grades.put(grade >= 71 && grade <= 80, Grade.B);
 grades.put(grade > 80 && grade <= 100, Grade.A);
 for (Map.Entry<Boolean, Grade> currentGrade : grades.entrySet()) {
 if (currentGrade.getKey() != true) {
 continue;
 }
 return currentGrade.getValue().name();
 }
 return "You must have been cheating";
}
enum Grade {
 F,
 D,
 C,
 B,
 A
}

Where I test it as:

@Test
public void gradeBy() throws Exception {
 int[] grades = {45, 52, 66, 93, 102};
 for(int grade : grades) {
 String gradeString = this.grading.gradeBy(grade);
 System.out.println(grade + "gives :"+ gradeString);
 }
}

Can this be improved in a more OOP way? Can the grading list be apart from the grading system it self?

Answer 1

 TreeMap<Boolean, Grade> grades = new TreeMap<>();
 grades.put(grade <= 50, Grade.F);
 grades.put(grade >= 51 && grade <= 60, Grade.D);
 grades.put(grade >= 61 && grade <= 70, Grade.C);
 grades.put(grade >= 71 && grade <= 80, Grade.B);
 grades.put(grade > 80 && grade <= 100, Grade.A);
 for (Map.Entry<Boolean, Grade> currentGrade : grades.entrySet()) {
 if (currentGrade.getKey() != true) {
 continue;
 }
 return currentGrade.getValue().name();
 }
 return "You must have been cheating";

You could rewrite the for loop as

 Grade letterGrade = grades.get(true);
 if (letterGrade != null) {
 return letterGrade.name();
 }

But this seems like an inefficient use of TreeMap. Consider

private static final NavigableMap<Integer, Grade> grades = new TreeMap<>();
static {
 grades.put(50, Grade.F);
 grades.put(60, Grade.D);
 grades.put(70, Grade.C);
 grades.put(80, Grade.B);
 grades.put(100, Grade.A);
}

This initializes once for the entire time the program is running.

And then in the method

 Map.Entry<Integer, Grade> letterGrade = grades.floorEntry(grade);
 if (letterGrade == null) {
 return "You must have been cheating";
 }
 return letterGrade.getValue().name();

This makes use of the NavigableMap ordered properties to binary search the potential grades. Your original is more like an exceptionally inefficient, albeit compact, if/else structure.

Note that TreeMap is the implementation. NavigableMap is the interface. You almost always want to set the variable type as the interface rather than the implementation. This allows for easy changes between implementations and encourages you to restrict yourself to the capabilities of the interface.

Answer 2

What stands out to me is that you initially perform a check against every grade range and insert true into the map in the right one. By that point, you've already found which grade is right. But then you initiate another loop to start searching for the grade corresponding to true.

I think instead, you ought to alter your method to output a Grade enum, and delegate the string handling to another function. I feel that it's better practice to return a Grade Enum from the first method because of the strong typing-- you may want to use this information for other methods besides finding the grade name. In fact, Enums are particularly nice because you can use a switch on them.

public Grade gradeBy(int grade) {
 if (grade < 50) return Grade.F;
 if (grade < 61) return Grade.D;
 if (grade < 71) return Grade.C;
 if (grade < 81) return Grade.B;
 if (grade<=100) return Grade.A;
 return Grade.F;
}
public String gradeName(Grade g) {
 return g.name()
}

Notice how as soon as the grade matches the range corresponding to something in the Grade Enum, the method immediately returns. Now an example of how you might use that switch:

public String gradeComment(Grade g) {
 switch(g) {
 case F:
 return "You need to work harder!";
 case D:
 return "I think you can do better.";
 case C:
 return "Satisfactory.";
 case B:
 return "Nice job.";
 case A:
 return "Well done!";
 default:
 return "N/A";
 }
}

Not that you need these grade messages, but that's an example of the power of holding on to the Enum type rather than converting the int grade directly into a String.

Sorry if I've made syntax errors as I've not used Java in a while.

Edit: Just realised that you might like to put gradeBy, gradeName and gradeComment into the Grade Enum itself as static methods.

Answer 3

Enums in Java are lovely. You could expand your enum to include the required score to get it:

enum Grade {
 F(0),
 D(51),
 C(61),
 B(71),
 A(81),
 CHEATER(101);
 private final int requiredScore;
 private Grade(int requiredScore) {
 this.requiredScore = requiredScore;
 }
 public int getRequiredScore() {
 return this.requiredScore;
 }
}

As you have a limit on 100 points, I added the special CHEATER grade.

Then to find out a score, you can loop through the scores and find the best matching score:

public static Grade gradeForScore(int score) {
 return Arrays.stream(Grade.values())
 .filter(grade -> score >= grade.getRequiredScore())
 .max(Grade::ordinal).get();
}

The above is using Java 8 and might be a bit over your knowledge level. I strongly recommend you to implement your own approach for finding the correct score. (Use the tips from this answer but make it a loop over all the possible grade values)

---

Your test is not really a test because you are not using any assert functionality. Use org.junit.Assert.assertEquals to make sure that the results are what you expect. In the future you might also be interested in using a Parametrized test

• Brendan Lesniak: I wouldn't be getting the optional directly with a call to .get(). You should instead opt for the safer .orElse(...) construct so you can specify a return value when the optional is not present. Although, this shouldn't be the case here as you should always have a max (unless your stream was null).
• corsiKa: I don't think we should encourage people to use ordinal for anything business related. It's too easy for a junior dev to change the order accidentally. The Grade enum should keep its individual min and max values and return that returns exactly one value from the filter.
• yitzih: Wouldnt you want to dofilter(grade -> grade.getRequiredScore() <= score) (as opposed to >= because you want the highest score that is lower than the current grade (ie. if the student has an 79 you want the grade for 71 not for 81)
• Simon Forsberg: @yitzih Whoops. Fixed that, although I'm keeping the >= ;)

Answer 4

Something to note is that when it comes to extending the program, or adapting it for another use, your grade boundaries are fixed in code which means changing or adding new grade boundaries would require a code change.

I'd recommend creating a GradeBoundary class with two variables specifying the upper and lower bound for the grade, and a String with the grade letter saved. These get saved into a collection and looped though using something like this:

For (GradeBoundary b : gradeBoundaries) {
 if(b.getLowerBound() <= b && b < b.getUpperBound()) {
 return b.getGrade()
 }
}

You'd also want a way, in your front end of supplying grade boundaries, either via XML, flat file, or System.in at a push. You want to keep this separate from the 'Business Logic' of the program as much as possible, to allow the backend to be migrated where possible.

• Simon Forsberg: Welcome to Code Review. Just wanted to say that this is good advice. Indeed it's more preferable to be able to read data from a file rather than hard-coding it!