Computing sum and counting matches in an array

Question

The problem was pretty straightforward and used two simple algorithms from our examples; the one for computing the sum, and the one for counting matches. The array for the problem wasn't provided, but you can easily write a method around it without actually having it, since she gave us the name and type of the array. The code for my solution is this:

 public class Tests

 {

 private double[] grades;

 public int belowAverage() // Creates method for finding scores below the mean

 {

 double total = 0;

 for (double element1 : grades) // Computes total of array 'grades'

 {

  total=total + element1;

 }

  double mean = total / grades.length; //Uses total and the length of the array to computer the mean



  int matches = 0;

  for (double element2 : grades) //Uses mean to compute number of grades that are below the mean and 

   {                                               // increases matches for each one that is.

    if (element2 < mean) {matches++;}

 }

 return matches; // Returns the number of matches.

  }

  }

Obviously the third line would change if you had the actual array. That was just kind of a placeholder. I tried to notate the code well enough to see where you get the total, then the mean, then compare each one to the mean to get matches that are less and increase your 'matches' variable for each match. I could have used the same 'element' variable for both loops since they're only used within the loops, but for clarity's sake in this example, I named them differently. I put all of these steps together in one method to solve this problem, but if I were going to write this sort of thing 'for real', I would assume it would probably need to calculate much more than just this small task (different averages, highest/lowest, etc.), so I would probably put each step in its own method so it could be used wherever you needed it.

Can someone comment on this and tell me if I could have done this in another way or an easier way? Maybe I could have used arrays.

Answer 1

1. From Steve McConnell, Code Complete 2nd Edition, 31.2 Layout Techniques, p737:

   Using blank lines is a way to indicate how a program is organized. You can use them to divide groups of related statements into paragraphs, to separate routines from one another, and to highlight comments.

   It does not make sense if all groups contain only one line.

2. This comment is mostly noise:

   return matches; // Returns the number of matches.
   

   as well as this one:

   // Uses total and the length of the
   // array to computer the mean
   double mean = total / grades.length;
   

   They don't say anything more that the code already does. I'd remove them. (Clean Code by Robert C. Martin: Chapter 4: Comments, Noise Comments)

3. I would call both element1 and element2 as element. It's more common. Having element1 and element2 suggest a little bit that they are used together (but that's not the case).

4. Comments like this are not too easy to read on because of the horizontal scrolling and the unnecessary spaces:

   {                                               // increases matches for each one that is.
   

   You could put them a line above of the method/loop/variable declaration (and eliminate them later).

5. [...] if I were going to write this sort of thing 'for real', I would assume it would probably need to calculate much more than just this small task (different averages, highest/lowest, etc.), so I would probably put each step in its own method so it could be used wherever you needed it.

   You should to that now. It would eliminate most of the comments and increase the abstraction level of the method. A maintainer would be grateful, it's easier to get an overview what the method does (without the details) and you can still check them if you are interested in. Furthermore, you need to read and understand less code (not the whole original method) if you modify just a small part of it.

   Finally, cleaning the code usually costs less during writing than later. Now you probably know the details better than later, therefore it's faster.

   public int scoresBelowMean() {
       double total = getGradesTotal();
       double mean = total / grades.length;
       return gradesCountBelowMean(mean);
   }
   
   private double getGradesTotal() {
       double total = 0;
       for (double element: grades) {
           total = total + element;
       }
       return total;
   }
   
   private int gradesCountBelowMean(double mean) {
       int matches = 0;
       for (double element: grades) {
           if (element < mean) {
               matches++;
           }
       }
       return matches;
   }
   

   (Clean Code by Robert C. Martin, One Level of Abstraction per Function, p36)

6. Instead of doubles (and floating point numbers) you might want to use BigDecimals. Floating point numbers are not precise. Here is an example:

   • Effective Java, 2nd Edition, Item 48: Avoid float and double if exact answers are required
   • Why not use Double or Float to represent currency?