Why does this code not work as intended?

Question

I'm writing a simple method that takes grades as input from the user and calculates the Grade Point Average. Here is my code:

public static double calculateGPA(){
   Scanner in = new Scanner(System.in);

   double totalGradePoints = 0; // total grade points
   int numClasses = 0; // total classes completed
   boolean doneInput = false; // returns true when use is done inputting grades

   System.out.println("Enter all your grades (A,B,C,D,F) and enter 'done' if you are done entering your grades.");  
   while (!doneInput) {     
      String grade = in.next();

      if (grade == "A") {
         totalGradePoints += 4;
         numClasses++;
      } else if (grade == "B") {
         totalGradePoints += 3;
         numClasses++;
      } else if(grade == "C") {
         totalGradePoints += 2;
         numClasses++;
      } else if(grade == "D") {
         totalGradePoints += 1;
         numClasses++;
      } else if(grade == "F") {
         numClasses++;
      } else {
         doneInput = true;
      } //end if - else-if - else statement
   }//end while loop
   double unwtGPA = (totalGradePoints/numClasses);
   return unwtGPA;
}

When I tested the method, I was only able to input one grade and none of the variables incremented, can somebody tell me what's wrong with the code?

Answer 1

The issue is with the string comparison using == instead of equals. == compares the references, which are very unlikely to be equal. Change to

 if(grade.equals("A")){
        totalGradePoints += 4;
        numClasses++;
    }else if(grade.equals("B")){ ...

and it should work. See this answer for a detailed explanation.

As a good practice it is advisable to always use the static string as the object for calling equals on to prevent a NPE:

 if("A".equals(grade)){
        totalGradePoints += 4;
        numClasses++;
    }else if("B".equals(grade)){ ...

If you are using Java 7, you can also do a switch statement with strings (though this one will throw an NPE if grade is null):

switch(grade) {
    case "A":
        totalGradePoints += 2;
        numClasses++;
        break;
    case "B":
        ...
}

And finally, since you are converting only one letter to an integer, to best solution is to convert them to char and for the values between A and D to do totalGradePoints += ('D' - grade.charAt(1)) + 1. So something along those lines would be simplest to read IMO:

while (true) {        
    final String input = in.next();
    if(input == null || input.isEmpty())
        break;

    final char grade = input.charAt(0);
    if(grade >= 'A' && grade <= 'D') {
        totalGradePoints += ('D' - grade) + 1;
    } else if(grade == 'F') {
        // no gradepoints for 'F'
    } else {
        break;
    } //end if - else-if - else statement

    ++numClasses;
} //end while loop

Answer 2

The problem is that you are using == to compare Strings, when you should be using s.equals("..."). The == operator checks identity and Strings are not guaranteed to have the same identity (there may be several objects representing exactly the same string). The equals() method compares the contents of the Strings for equality rather than the identity of the objects.

I would write it more like this:

Scanner in = new Scanner(System.in);

double totalGradePoints = 0; // total grade points
int numClasses = 0; // total classes completed

System.out.println("Enter all your grades (A,B,C,D,F) and"
        + " write 'done' when you have finished");

while (true) {
    String input = in.next();
    if (input.equals("done")) {
        break;
    }
    if (input.equals("A")) {
        totalGradePoints += 4;
    } else if (input.equals("B")) {
        totalGradePoints += 3;
    } else if (input.equals("C")) {
        totalGradePoints += 2;
    } else if (input.equals("D")) {
        totalGradePoints += 1;
    } else if (!input.equals("F")) {
        System.err.println("Invalid input: " + input);
        continue;
    }
    numClasses++;
}
double unweightedGPA = (totalGradePoints / numClasses);
System.out.println(unweightedGPA);

The while(true) loop with break (to exit the loop) and continue (to skip back to the start of the loop) is more idiomatic Java. The main benefit with my version above is that there is no duplication of the line numClasses++.

You could even make the loop more succinct and make it simpler to alter the grade points by using a Map, which will return a points value for the grade or null for an invalid grade:

Map<String, Integer> gradePoints = new HashMap<String, Integer>() {{
    put("A", 4);
    put("B", 3);
    put("C", 2);
    put("D", 1);
    put("F", 0);
}};

String input;
while (!(input = in.next()).equals("done")) {
    Integer points = gradePoints.get(input);
    if (points == null) {
        System.err.println("Invalid input: " + input);
        continue;
    }
    totalGradePoints += points;
    numClasses++;
}

It's also worth pointing out that in your code and in all the answers on this page, if you don't enter any grades, you will get a division-by-zero (yielding NaN), so you might want to think about handling that gracefully.

Answer 3

Your failure is to use the == operator to check Strings on equality. Strings are objects so in this case there will be a check on object identity (which defaults to a reference check).

Use equals to make a character by character check like so:

if(grade.equals("A")){