-15

I need to match the average score to a letter grade. That means

if (90 < avg && avg < 100) {
     return 'A';
} 

and so on until 'F', with 5 if-else statements.

That's a lot of repetition, and the ranges I'm matching to are of the same length.

Is there a more efficient way to do this? I don't want to repeat the if-else statement 5 times.

NoDataDumpNoContribution
  • 10,591
  • 9
  • 64
  • 104
L C
  • 67
  • 7
  • 2
    Integer division by 10, and then use a `switch`. – Robby Cornelissen Aug 15 '19 at 13:32
  • Create an array of grades. (100 - avg) / 10 is the index of the grade you want (unlest it's larger than the last valid index, in which case you just want to return the last index (assuming the ranges are in fact [91-100], [81-90], etc.) – JB Nizet Aug 15 '19 at 13:34
  • You **could** probably write some heuristic functions that maps an Integer in between 0 and 100 to a specific character without using any conditionals. But that would not really be worth the effort. Other than that your solution is already the most "efficient" solution if you define it as "having the least lines of code". – T A Aug 15 '19 at 13:35
  • @LutzHorn I tried adding code, but it won't format correctly (I'm new to stackoverflow). The instructions say to type two whitespaces for line break but that never worked... – L C Aug 15 '19 at 14:03
  • 8
    This question is being discussed on [meta](https://meta.stackoverflow.com/questions/388573/the-question-was-closed-as-unclear-what-you-are-asking-why) – Samuel Liew Aug 16 '19 at 06:59
  • Good question and definitely not too broad. If it were too broad, then almost any question on SO would be. Just have a look at the quality of the answers. – NoDataDumpNoContribution Aug 16 '19 at 10:31
  • 1
    @Trilarion Are grades always integers?, The `if`statement will never catch multiples of 10 as it stands, is this intended?, is 90 an 'A' or a 'B'?, What about 100? Is that a higher grade than an 'A' or is it an 'A'? What's wrong with `if` statements? What kind of answer, a complex performant one? One that's easy to understand? the list goes on... – Nick is tired Aug 16 '19 at 10:46
  • @NickA Grades seem to always be letters from A to F, scores seem to be integers. The if statement has problem with multiples of 10; that could be discussed in an answer. Repeated if statements are surely a code duplication and you could get things wrong, the asker already got the multiples of 10 wrong. He probably wants to avoid making such mistakes. Performance is almost never a concern for people in such cases. You're not wrong and the question could be improved, but I think the answers are already useful. – NoDataDumpNoContribution Aug 16 '19 at 10:56
  • 1
    Please see also [Using java map for range searches](https://stackoverflow.com/questions/1314650/using-java-map-for-range-searches) or [Using switch statement with a range of value in each case?](https://stackoverflow.com/questions/10873590/in-java-using-switch-statement-with-a-range-of-value-in-each-case). – NoDataDumpNoContribution Aug 16 '19 at 11:00
  • 2
    @NickA One trick that SO has made in such cases is adapting the question to the answers, no matter who does it. It would be a pity if useful answers are disregarded, just because an asker got it not right. – NoDataDumpNoContribution Aug 16 '19 at 11:08

7 Answers7

6

I like to use an enum for this kind of problem.

You simply define your ranges in the enum values and use a predicate.

import java.util.*;

public class MyClass {
    public static void main(String args[]) {
      System.out.println(Letter.getValue(13)); // B
      System.out.println(Letter.getValue(28)); // C
      System.out.println(Letter.getValue(43)); // empty
    }
}

enum Letter {

    A(1, 10), B(11, 20), C(21, 30);

    int min;
    int max;

    Letter (int min, int max) {
        this.min = min;
        this.max = max;
    }

    boolean inRange(int v) {
        return v >= min && v <= max;
    }

    static Optional<Letter> getValue(int i) {
        return Arrays.stream(values())
           .filter(v -> v.inRange(i))
           .findAny();
    }

}
Arnaud Claudel
  • 3,000
  • 19
  • 25
4

You may find this approach verbose and overengineered. Yes, it's.

Though, I like the way the grading system is defined.

class Test {
  public static void main(String[] args) {
    Map<String, Predicate<Integer>> gradingSystem = new HashMap<>();

    gradingSystem.put("A", mark -> mark >= 90 && mark <= 100);
    gradingSystem.put("B", mark -> mark >= 80 && mark < 90);

    translateScoreIntoGrade(86, gradingSystem); // B
    translateScoreIntoGrade(95, gradingSystem); // A
  }

  public static String translateScoreIntoGrade(int score, Map<String, Predicate<Integer>> gradingSystem) {
    return gradingSystem.entrySet().stream()
        .filter(gradePredicate -> gradePredicate.getValue().test(score))
        .findFirst()
        .map(Map.Entry::getKey)
        .orElseThrow(() -> new IllegalArgumentException("This grade isn't valid for this system!"));
  }
} 
Andrew Tobilko
  • 48,120
  • 14
  • 91
  • 142
  • I prefer the `90 <= mark && mark <= 100` notation (order) – user85421 Aug 15 '19 at 13:46
  • 1
    @Michael I think the difference here is the (non)usage of external libraries - personally, that's why I prefer this answer. As for the asker, I don't know. – Avi Aug 15 '19 at 14:02
  • 1
    Well, there is still some duplication while writing the predicates – Arnaud Claudel Aug 15 '19 at 15:22
  • 1
    duplication could be avoided using a sorted map, but it is very flexible since the *rules* can be as complex as needed (and if put inside a small class, you could have different grading systems) – user85421 Aug 15 '19 at 15:32
  • @ArnaudClaudel in my opinion, the approach you're promoting isn't flexible enough (particularly, the method `inRange`). What if A is only 100? What if B is from 80 to 90 *and* with the correct answer on X in the exam? Conditions could vary greatly. – Andrew Tobilko Aug 16 '19 at 08:31
  • True, but if conditions can vary, then you won't have duplication. Here it's always the same predicate – Arnaud Claudel Aug 16 '19 at 08:34
  • @ArnaudClaudel I assume by duplication, OP meant repeating `if-else` statements, not the conditions they hold – Andrew Tobilko Aug 16 '19 at 08:36
  • Yes probably. And you're definitely right, my solution would require some refactor if the conditions can be something else than `inRange`. `Predicate` is better – Arnaud Claudel Aug 16 '19 at 08:43
3

You could do it like this example:

public static String avgValuetoGrade(int value) {
        int index = value/10;
        int gradeIndex = Math.max(0, index - 4);
        return (char)('F'-gradeIndex );
    }

First divide you avg by 10, you get the number 0-9 (integer division). Next you have to reduce the numbers 0-4 somehow to one number -4 gives you the numbers 0-5 and maps the 0-4 to 0. Then you can return the char (thanks to @Carlos Heuberger).

tobsob
  • 602
  • 9
  • 22
  • Using an array of grades and getting the grade by its index in the array would be simpler than using a switch statement. – JB Nizet Aug 15 '19 at 13:41
  • no `switch` needed, `(char)('F'-num)` would do (but no error detection) - and very *enigmatic*, but all the math is too – user85421 Aug 15 '19 at 13:41
  • could use `Math.abs(div - 4)` instead of `Math.max(0, div - 4)` – Avi Aug 15 '19 at 13:44
  • 4
    It covers OP's needs but is tricky and not universal – Nikolai Shevchenko Aug 15 '19 at 13:46
  • I downvoted for 2 reasons: 1) you changed the approach completely, 2) the latest approach is very hard to read and maintain. Undid my vote. – Andrew Tobilko Aug 15 '19 at 13:51
  • you could go with a one-liner `return (char)('F' - Math.max(0, value / 10 - 4))`, but would it be ok? – Andrew Tobilko Aug 15 '19 at 14:13
  • big problem: maintenance, if the requirement changes to a different range, this will be hard to change - but it's what was asked for. I, for production code, would prefer the simple ìf`-`else` *tree* – user85421 Aug 15 '19 at 14:31
  • 1
    What if the requirements changes and we want to reture a grade from `1` to `5` ? Your implementation and logic are tied to the letters `A-F` – Arnaud Claudel Aug 15 '19 at 15:18
2

This would be an example of the same thing written in a functional style. There is no repetition, but it's quite verbose.

I wouldn't use it unless the bands were different sizes (where you can exploit integer division)

Map<Range<Integer>, Character> rangeToGrade = Map.of(
   Range.between(90, 100), 'A',
   Range.between(80, 90), 'B'
   //... the rest
);

int mark = 50;
char grade = rangeToGrade.entrySet().stream()
    .filter(e -> e.getKey().contains(mark))
    .map(Map.Entry::getValue)
    .findFirst()
    .orElse('?'); // or throw an exception

Map.of from Java 9. Range from e.g. Apache Commons

Michael
  • 41,989
  • 11
  • 82
  • 128
  • 3
    That's longer, harder to read, and less efficient than 6 if/else, IMHO. And it needs a large dependency, too. – JB Nizet Aug 15 '19 at 13:44
  • From my point of view this is kinda best solution. External dependency (Range) can be replaced with own simple class – Nikolai Shevchenko Aug 15 '19 at 13:48
  • The point is to use code with less repetitions, and be more efficient. How does your code hit the point? – JB Nizet Aug 15 '19 at 13:48
  • @JBNizet The only repetition is the definitions of the ranges and, as I said, I would only use this if the ranges could not be genericized. So in such as case, the contains the minimum amount of repetition that is required. Efficiency... Without measuring is premature optimization anyway and I know that you know that. – Michael Aug 15 '19 at 13:55
  • 1
    Sure I know that. My point is that this has exactly the same number of repetitions as the solution using if/else, has additional code in addition to those repetitions, is not more efficient, and is, IMHO, less readable. You might disagree with me, but Idon't see how I'm missing the point. – JB Nizet Aug 15 '19 at 14:02
  • 1
    @JBNizet No, it has fewer repetitions. The check to see whether a mark is within a grade is on a single line. I don't have to worry about accidentally writing a `<` instead of a `>` and breaking everything. – Michael Aug 15 '19 at 14:07
  • @JBNizet There is still repetition because you need to declare your range at one point. However this is way more elegant than if / elsef and it's not really the same kind of repetition. – Arnaud Claudel Aug 15 '19 at 14:09
1

You can map it to an array and avoid the branch prediction rules, thereby making it more efficient and also saving the if/else stuff.

class StackOverflow
{
    public static void main(String args[])
    {
        char grades[]={'A','B','C','D','E','F'};
        convert(grades,95);
        convert(grades,90);
        convert(grades,110);
        convert(grades,123);
        convert(grades,150);
    }
    static void convert(char grades[],int marks)
    {
        if(marks<=90||marks>=150)
        {
            System.out.println("Invalid marks");
            return;
        }
        int val=marks/10-9;
        System.out.println(grades[val]);
    }
}

Note: This assumes that 91-99 is A, 100-109 is B, 110-119 is C and so on. If you want to avoid the numbers 100,110 etc. just add the rule ||marks%10==0 in the if statement above.

Hope this is helpful :)

Aditya Pal
  • 61
  • 2
  • 5
1

Just to show off, in Java version 12 (preview enabled), using the new Switch Expressions:

String grade = switch(avg/10) {
    case 9,10 -> "A";
    case 8    -> "B";
    case 7    -> "C";
    case 6    -> "D";
    case 5    -> "E";
    default   -> "F";
};

or, very flexible, if not being to lazy:

String grade = switch(avg) {
    case 90, 91, 92, 93, 94, 95, 96, 97, 98, 99, 100 -> "A";
    case 80, 81, 82, 83, 84, 85, 86, 87, 88, 89      -> "B";
    // you got the idea, I AM lazy

Real (?) solution: use NavigableMap (example TreeMap) with its floorEntry() or lowerEntry() methods:

NavigableMap<Integer, String> grades = new TreeMap<>();
grades.put(90, "A");
grades.put(80, "B"); 
...
// usage
String grade = grades.floorEntry(avg).getValue();

values in map must eventually be adjusted

user85421
  • 28,957
  • 10
  • 64
  • 87
  • @AndrewTobilko agreed, at least about it being terrible, but eventually the fastest (just a jumptable, compares to normal switch {I believe}) – user85421 Aug 15 '19 at 14:27
1

Note: This is made using Java 8 and does not have access to MapOf and does not use external libraries. Also did not use Streams to show other options.

I made a GradeBook class that when it is instantiated, a class field for a Map is filled with the keys used to find the letter grade. You can then just call .getGrade(int) from your GradeBook object with will also handle cases of negative input and input above 100 with a return of N. Otherwise it will return the correct grade from the Map.

This is more of an Object Oriented approach rather than using a static method call:

public class GradeBook {

    private Map<Integer, Character> map;

    public GradeBook() {
        constructMap();
    }

    public char getGrade(int grade) {
        if (grade >= 0 && grade <= 100) {
            return map.get(Math.max(0, grade/10 - 4));
        }
        else {
            return 'N';
        }
    }

    public void constructMap() {
        //You can use MapOf to add all of them at once in Java9+
        map = new HashMap<>();
        map.put(0, 'F');
        map.put(1, 'E');
        map.put(2, 'D');
        map.put(3, 'C');
        map.put(4, 'B');
        map.put(5, 'A');
        map.put(6, 'A');
    }

    public static void main(String [] args) {

        GradeBook grades = new GradeBook();

        //Testing different values
        System.out.println(grades.getGrade(100));
        System.out.println(grades.getGrade(75));
        System.out.println(grades.getGrade(80));
        System.out.println(grades.getGrade(91));
        System.out.println(grades.getGrade(45));
        System.out.println(grades.getGrade(2));
        System.out.println(grades.getGrade(-1));
    }
}

Output:

A
C
B
A
F
F
N

The downside to this implementation is it is slightly long to implement the first time, but the upside is it is very easy to reuse as you just need to create a new GradeBook() anywhere you need it.

Nexevis
  • 4,647
  • 3
  • 13
  • 22