A refactoring example: Grade reporter

Section 6.6 A refactoring example: Grade reporter

In this section we will do an initial refactoring of the little grade-report program we worked on earlier. For reference, here is the code that we ended up with:

private static String processGrades(Scanner scanner) {
  int n = 0;    // number of courses
  double t = 0.00; // total gpa
  while (scanner.hasNext()) {
    scanner.next();             // The course prefix
    scanner.next();             // The course number
    String lg = scanner.next(); // The letter grade
    if (!lg.equals("W")) n += 1;
    switch (lg) {
      case "A":
          t += 4.00;
          break;
      case "A-":
          t += 3.67;
          break;
      case "B+":
          t += 3.33;
          break;
      /// A few more cases here
      case "D":
          t += 1.00;
          break;
      case "D-":
          t += 0.67;
          break;
      default:
    }
  }
  double gpa = n == 0 ? 0.00 : t / n;
  return String.format("Courses: %d%nGPA: %.2f%n", n, gpa);
}

There’s a whole lot of cleanup that we need to do to this function. We will focus on two items first and foremost:

Better names for the local variables (renaming refactoring)
Extracting a lot of methods to provide more explanatory names for parts of the algorithm.

Subsection 6.6.1 Cleaning up variable names

Let’s start with variable names. n was supposed to stand for the number of courses. We could use numberOfCourses, but that’s a bit long. courseNo sends the wrong message (the number of the course as opposed to the number of courses). I will go with numCourses. num is a clear enough abbreviation for number of courses. Therefore I’ll perform a rename refactoring on n and turn it into numCourses. Then I will remove the now unnecessary comment next to the declaration.

Next we have t. That’s the total points allocated from adding up all the course grade points. I will rename it to totalPoints and remove its comment. Then we have lg, which stands for "letter grade", so I will stop trying to be cute and just say letterGrade instead, and remove the comment.

Lastly we have gpa. That sounds like a fine enough name for it. I could rename it to gradePointAverage, but I think we can assume everyone is familiar with the abbreviation. So at this point here is how our code looks:

private static String processGrades(Scanner scanner) {
  int numCourses = 0;
  double totalPoints = 0.00;
  while (scanner.hasNext()) {
    scanner.next();   // The course prefix
    scanner.next();   // The course number
    String letterGrade = scanner.next();
    if (!letterGrade.equals("W")) numCourses += 1;
    switch (letterGrade) {
      case "A":
        totalPoints += 4.00;
        break;
      case "A-":
        totalPoints += 3.67;
        break;
      case "B+":
        totalPoints += 3.33;
        break;
      // More cases here
      case "D+":
        totalPoints += 1.33;
        break;
      case "D":
        totalPoints += 1.00;
        break;
      case "D-":
        totalPoints += 0.67;
        break;
      default:
    }
  }
  double gpa = numCourses == 0 ? 0.00 : totalPoints / numCourses;
  return String.format("Courses: %d%nGPA: %.2f%numCourses", numCourses, gpa);
}

Subsection 6.6.2 Introducing explanatory methods

So far so good. Still, a very long function, I would really like to break it up into smaller pieces. But the next thing I want to do is introduce some explanatory methods for steps that I took, especially steps that I felt the need to document with a comment next to them.

Let’s start with the first scanner.next();. That step is meant to read the course prefix. I will extract that line into a method called readPrefix, and remove the comment.

Now right after it there is another scanner.next();. Should I use readPrefix twice? No, even though this is the same piece of code, it fulfills a very different function, namely reading the course number. Therefore I will extract it to a method called readCourseNo. This is an important lesson:

Before removing code duplication by using an extracted method, make sure both pieces of code serve the same purpose in your code, and that they are not the same by accident.

Lastly, we have one more scanner.next on the right-hand-side of the letterGrade initialization. I will extract that to readLetterGrade, and that one returns a string.

Thinking about it more, the three lines of code I just extracted into three separate methods actually make sense as a whole: Read the next line and return that letter grade. I will therefore group them up and extract a method from them, called processNextGradeRow. Here’s how the top of my function will look now:

private static String processGrades(Scanner scanner) {
  int numCourses = 0;
  double totalPoints = 0.00;
  while (scanner.hasNext()) {
    String letterGrade = processNextGradeRow(scanner);
    if (!letterGrade.equals("W")) numCourses += 1;

I acquired four new methods in the process, which I will place right below this method. This follows something called the stepdown rule, which introduces the big ideas early on and then elaborates them further and further as you go down the file, with the helper methods lined up in a natural order: To find the details about a method you look at the methods following it.

private static String processNextGradeRow(Scanner scanner) {
  readPrefix(scanner);
  readCourseNo(scanner);
  String letterGrade = readLetterGrade(scanner);
  return letterGrade;
}

private static String readLetterGrade(Scanner scanner) {
  return scanner.next();
}

private static void readCourseNo(Scanner scanner) {
  scanner.next();
}

private static void readPrefix(Scanner scanner) {
  scanner.next();
}

When I do that, I notice that my otherwise smart IDE created a local variable letterGrade in my processNextGradeRow method. That seems to me largely useless right now, so I will inline it and immediately return readLetterGrade(scanner);. Your IDE may even suggest that.

Subsection 6.6.3 Cleaning up the switch statement

We are far from done. Next I want to tackle that big switch statement. I would love to extract that into a separate method, perhaps something like "updateTotalForLetterGrade". Wordy, but remember that the smaller the scope of a function the longer its name can be, to explain what the function does to the few places it is needed. I need to be careful with it, as the function needs to use the totalPoints value and also return it. So the whole switch statement will end up being replaced with something like:

totalPoints = updateTotalForLetterGrade(totalPoints, letterGrade);

I will move this new method below the other methods I created earlier, since it appears later in the original function.

Now something bugs me. It feels weird to me that this new function needs to take in the totalPoints variable. But it needs to do so because of the totalPoints += ... lines. Thinking about it a bit, perhaps it would be nicer if I made this function return the points that the grade is worth, then do the addition back in the main function. I’ll need to rename the function too of course, but first things first. Here is how the function looks now:

private static double updateTotalForLetterGrade(double totalPoints, String letterGrade) {
  switch (letterGrade) {
    case "A":
      totalPoints += 4.00;
      break;
    case "A-":
      totalPoints += 3.67;
      break;
    // more cases
    case "D":
      totalPoints += 1.00;
      break;
    case "D-":
      totalPoints += 0.67;
      break;
    default:
  }
  return totalPoints;
}

I want to do this transition gradually, keeping the program running as much as possible. So I I will do it in small stages, at the end of each my program should be working:

Start by adding a totalPoints += 0; line in the default case.
Change all the + occurrences in the function body with =, change the = in the original call to this function to +=.
Replace all the totalPoints = ...; ... break; pairs of lines with a direct return .... I don’t need to be storing the values only to return them later, I can return them right away, and avoid the need for break. And I remove the return totalPoints line which is now unreachable.
Now I can rename this function to something like pointsForLetterGrade.
Next I eliminate the first parameter, totalPoints, which is no longer needed (and make sure to change the caller too).
I’ll put the return ... lines on the same line as the corresponding case ..., and line them up vertically, I like how that reads.

So here is what we have left:

private static double pointsForLetterGrade(String letterGrade) {
switch (letterGrade) {
  case "A":  return 4.00;
  case "A-": return 3.67;
  case "B+": return 3.33;
  case "B":  return 3;
  case "B-": return 2.67;
  case "C+": return 2.33;
  case "C":  return 2;
  case "C-": return 1.67;
  case "D+": return 1.33;
  case "D":  return 1.00;
  case "D-": return 0.67;
  default:   return 0;
}

Subsection 6.6.4 Levels of abstraction

So at this point the original function looks much simpler:

private static String processGrades(Scanner scanner) {
  int numCourses = 0;
  double totalPoints = 0.00;
  while (scanner.hasNext()) {
    String letterGrade = processNextGradeRow(scanner);
    if (!letterGrade.equals("W")) numCourses += 1;
    totalPoints += pointsForLetterGrade(letterGrade);
  }
  double gpa = numCourses == 0 ? 0.00 : totalPoints / numCourses;
  return String.format("Courses: %d%nGPA: %.2f%n", numCourses, gpa);
}

It’s still a bit too long, but also seems to be a bit unbalanced in terms of the level of abstraction of the different parts of it. Some are more "high level" like processNextGradeRow or pointsForLetterGrade, and others are more "low level". In general we want to keep our function body at the same level of abstraction.

Keep every part of the body of a function at the same level of abstraction.

I will start with the scanner.hasNext() part. I will extract a method for it called hasMoreEntries(scanner). It’s not perfect but at least a bit better.

Next there is a line that update the number of courses. I don’t like the lack of symmetry with the next line. I would like this line to also read: numCourses += .... We can do that if we have a function that returns 0 or 1 depending on whether the course counts for credit or not. I will do that by first rewriting the line using a ternary:

numCourses += letterGrade.equals("W") ? 0 : 1;

Now I will extract the right-hand-side to a function called creditsForLetterGrade.

Lastly, the last two lines are about calculating the gpa and then formatting the final result. I will extract each into methods calculateGPA and formatResult.

This looks quite nice to read now:

private static String processGrades(Scanner scanner) {
  int numCourses = 0;
  double totalPoints = 0.00;
  while (hasMoreEntries(scanner)) {
    String letterGrade = processNextGradeRow(scanner);
    numCourses += creditsForLetterGrade(letterGrade);
    totalPoints += pointsForLetterGrade(letterGrade);
  }
  double gpa = calculateGPA(numCourses, totalPoints);
  return formatResult(numCourses, gpa);
}

It still feels a bit too long, but without more major refactorings this is as far as we can go. It would be nice to extract the whole while loop into its own thing, or perhaps just its contents. Unfortunately that look changes 2 different things, numCourses and totalPoints. Since a method call can only affect the values of the caller’s variables via its return value, and methods can only return one value, we cannot possibly change two things. We will learn later of a workaround, where we turn those two things into one thing. But this is a good place to stop for now.

Prev Top Next