trjhtr

This is from a Java course which I'm taking. The class StudentUtil, including method-declarations and comments, was provided by the instructor. I just had to finish the bodies of the two methods.

calculateGPA() computes the average performance of students. Every A-grade gives 4 points, every B-grade 3 and every C-grade 2 point.

calculateGPA() returns students which have performed within a specified range.

The StudentUtil class:

package modul;

import java.util.ArrayList;

public class StudentUtil 

 public static double calculateGPA(int studentIdList, char studentsGrades) 
 double grades = new double[studentIdList.length];
 int i = 0;

 for (char studentGrade : studentsGrades) 
 double sumGrades = 0.0;

 for (char grade : studentGrade) 
 if (grade == 'A') 
 sumGrades += 4;
 else if (grade == 'B') 
 sumGrades += 3;
 else 
 sumGrades += 2;
 
 
 grades[i++] = sumGrades / studentGrade.length;
 

 return grades;
 

 public static int getStudentsByGPA(double lower, double higher, int studentIdList, char studentsGrades) 
 // perform parameter validation. Return null if passed parameters are not valid
 if (lower < 0.0) 
 return null;
 

 if (higher > 4.0) 
 return null;
 

 if (lower > higher) 
 return null;
 

 if (studentIdList.length != studentsGrades.length) 
 return null;
 
 // invoke calculateGPA
 double gpas = calculateGPA(studentIdList, studentsGrades);
 ArrayList<Integer> ids = new ArrayList<>(); 

 for (int i = 0; i < studentIdList.length; i++) 
 if (gpas[i] >= lower && gpas[i] <= higher) 
 ids.add(studentIdList[i]);
 
 
 // construct the result array and return it. You would need an extra for loop to compute the size of the resulting array
 int ret = new int[ids.size()];

 for (int i = 0; i < ret.length; i++) 
 ret[i] = ids.get(i);
 

 return ret;
 

My test class:

package modul;

import static java.lang.System.out;
import java.text.DecimalFormat;
import static java.lang.System.out;

public class Sandbox 

 public static void main (String args) throws Exception 
 int studentIdList = 1001, 1002 ;
 char studentsGrades = 'A', 'A', 'A', 'B' , 'A', 'B', 'B' ;

 double results = StudentUtil.calculateGPA(studentIdList, studentsGrades);

 for (double result : results) 
 out.printf("%.2fn", result);
 

 int ids = StudentUtil.getStudentsByGPA(3.2, 3.5, studentIdList, studentsGrades);

 for (int result : ids) 
 out.println(result);
 
 

It has passed the automated test.

But what do you think about the demand of returning null when the validation of getStudentsByGPA() fails? Would one really do that this way? Or would one throw some specific exception instead, IllegalArgumentException for example?

What do you think about my code in general? What would you have done else and why?

In order, with come code comments first:

The StudentUtil-class:

package modul;

Try to avoid naming things as misspelled words. "Module" would probably not be a good choice for a package name because "Module" has a meaning in programming in general, and since version 1.9 it even has a specific meaning in Java. This doesn't mean, however, that "Modul" is better.

import java.util.ArrayList;

public class StudentUtil 

 public static double calculateGPA(int studentIdList, char studentsGrades) 
 double grades = new double[studentIdList.length];
 int i = 0;

 for (char studentGrade : studentsGrades) 
 double sumGrades = 0.0;

 for (char grade : studentGrade) 

Double for loop, great, and without incrementing variables, even better. The names "grade" and "studentGrade" leave me a bit confused though. "studentGrade" contains more than one grade? I would replace "studentGrade" with something like "singleStudentGrades".

 if (grade == 'A') 
 sumGrades += 4;
 else if (grade == 'B') 
 sumGrades += 3;
 else 
 sumGrades += 2;
 

Consider having an explicit if (grade == 'C') case, and making your else case throw an error. It's a simple way to make sure your input is only ever A, B, or C. Unless we want to assume that anything not given a letter grade (e.g. something never assigned a grade) defaults to a 2.

 
 grades[i++] = sumGrades / studentGrade.length;
 

 return grades;
 

 public static int getStudentsByGPA(double lower, double higher, int studentIdList, char studentsGrades) 
 // perform parameter validation. Return null if passed parameters are not valid
 if (lower < 0.0) 
 return null;

Note about the nulls at the end of my answer

 

 if (higher > 4.0) 
 return null;
 

 if (lower > higher) 
 return null;
 

 if (studentIdList.length != studentsGrades.length) 
 return null;
 
 // invoke calculateGPA
 double gpas = calculateGPA(studentIdList, studentsGrades);
 ArrayList<Integer> ids = new ArrayList<>(); 

 for (int i = 0; i < studentIdList.length; i++) 
 if (gpas[i] >= lower && gpas[i] <= higher) 
 ids.add(studentIdList[i]);
 
 
 // construct the result array and return it. You would need an extra for loop to compute the size of the resulting array

I don't understand this comment - is it justification for calling ids.size()? If so, it's not quite right. You could add a counter variable that gets incremented in your if clause above. That doesn't mean that you necessarily should, though - see below

 int ret = new int[ids.size()];

 for (int i = 0; i < ret.length; i++) 
 ret[i] = ids.get(i);
 

Rather than this, consider List.toArray() - it'll save you the indexing. Check out this question: Converting 'ArrayList to 'String' in Java

 return ret;
 

test-class:

package modul;

import static java.lang.System.out;
import java.text.DecimalFormat;
import static java.lang.System.out;

public class Sandbox 

 public static void main (String args) throws Exception 
 int studentIdList = 1001, 1002 ;
 char studentsGrades = 'A', 'A', 'A', 'B' , 'A', 'B', 'B' ;

 double results = StudentUtil.calculateGPA(studentIdList, studentsGrades);

 for (double result : results) 
 out.printf("%.2fn", result);
 

 int ids = StudentUtil.getStudentsByGPA(3.2, 3.5, studentIdList, studentsGrades);

 for (int result : ids) 
 out.println(result);
 
 

I encourage you to consider writing unit tests (I recommend JUnit). It'll make your tests easier to write and read. The above tests are ok, but I would definitely add tests to check that you get null when you expect to.

Overall:

Your code is overall quite clean and easy to follow. It's not quite production-level, but it is good for a Java course. Nice job!

Regarding returning null:

There is a large discussion around this issue. I think most people believe something along these lines: 1.) null should not be used in place of an error. This is because in general, methods should enforce their contracts loudly, i.e. if a method's job is to take a number greater than zero and return it times two, and it gets a number less than zero, it should complain. By returning null, you force programmers who use your code (if they're responsible) to write in code to handle the possible null case, because otherwise there's no guarantee that they will be aware of the error that made your method return null.

And 2.) null can be used as "no results", but it might be better to return an Option or an empty data structure instead. For more, check out this question Is it better to return NULL or empty values from functions/methods where the return value is not present?

For a class assignment, though, it may well be the case that using null is "ok" because its fast - and it doesn't require you to shift focus from whatever the point of this assignment is to spend time writing exception throwing code. Avoiding using null as an error, and instead throwing an exception, is a best practice, though, and as you grow as a programmer you may want to do so even if it's not required. Be careful though - if your code is auto-graded and it is expected that it will return null for a given input, an exception will probably cause it to be marked wrong. In this case I suggest writing code to your assignment's specification, and being content with knowing that were you writing code in the "real world", you'd do it differently

First, about your code:

• You hard-coded the points associated with each grade into both methods calculateGPA and getStudentsByGPA (in getStudentsByGPA, you defined the upper boundary as 4.0, probably because this is the number of points associated with the best grade), thereby making them magic numbers. If the number of points associated with the grade A changed, or if the available grades themselves changed, you would have to rewrite both methods. A more robust approach would be to define the points associated with a grade in one place, and then refer to this place when you need to query any information related to the association of points with grades. For example, you could create a Map<Character, Integer> that maps each grade to the number of points associated with it, and wenn you need to get the maximum number of points, you can call Map.values().stream().max(Comparator.naturalOrder()).


• Regardless of the above, in the method getStudentsByGPA, I wouldn't consider an upper boundary larger than the maximum number of points for a grade, or a lower boundary less than zero, as invalid. While it's not possible for a student to have an average less than $0$ (in fact, the average can not even be less than $2$, at least if the student has any grades at all – if a student doesn't have any grades, the average will correctly be stored as Double.NaN as a result of a zero-divided-by-zero operation – I'm just pointing this out in case you were not aware of this edge case) or greater than $4$, it would still make sense to ask for all students with an average between $-1$ and $5$. You could even ask for all students with an average between $39.7$ and $44.235$ – it's just that there are no students that can possibly satisfy this criterion. But this doesn't make the criterion invalid.


• On the other hand, a boundary of Double.NaN would indeed be invalid, but a NaN would slip through your argument checks due to the way Double.NaN behaves with comparison and equality operators (which is specified in Chapter 15 of the Java Language Specification in the sections 15.20.1 and 15.21.1). If you were to keep the boundary checks of the parameters, then you could rewrite them from if (lower < 0.0) to if (!(lower >= 0.0)), and from if (higher > 4.0) to if (!(higher <= 4.0)). That way, if one boundary is Double.NaN, the conditions will evaluate to true and the method returns null, as required. If not, and you want to redesign the code to simply check whether a boundary is Double.NaN, you should be aware that Double.NaN == Double.NaN returns false, so you cannot do if (lower == Double.NaN). Instead, there is a method Double.isNaN(double) exactly for this purpose. But, come to think of it, you need to check whether lower is less than or equal to higher anyway, so even if you didn't check whether the two boundaries are between 0 and 4, you can still implicitly include the NaN check in if (!(lower <= higher)).


• Another possibility of invalid arguments is studentIdList, studentsGrades, or any sub-array of studentsGrades being null. You neglect to check this (although the validity of the sub-arrays of studentsGrades should be ascertained by calculateGPA and not by getStudentsByGPA, since getStudentsByGPA only cares about the length of studentsGrades when it compares it to the length of studentIdList, but not about its sub-arrays).

Now, I also have some points of criticism on the code design prescribed by your instructor, which I'm going to mention here as well for the sake of completeness.

• The parameter studentIdList in the method calculateGPA(int, char) is useless. The method does not need it. Granted, you use it, but just to get the number of students, and you could get that from the parameter studentsGrades as well. There is absolutely no reason why this method should accept this parameter. Passing more data to a method than necessary makes code confusing, because it suggests that the behavior of a method depends on more state than it actually does.


• 
  

  In the method getStudentsByGPA, the parameter studentIdList is indeed necessary, but making studentIdList and studentsGrades two independent paramaters when they are in fact related to each other makes the code fragile and difficult to handle, as you yourself noticed, not least because it requires such annoying checks as whether the lengths of the two arrays match. An easier design would be to replace the two arrays with a Map<Integer, char>. That way, the relationship between a student id and an array of grades is implicitly reflected by the code design, instead of being a coincidence the compiler cannot know anything about. This would also give numerous other advantages:

  
  
  
  • An int can contain duplicates. So theoretically, with your instructor's design, the method getStudentsByGPA could be passed garbage input in the form of a studentIdList that contains duplicates, in which case it has the choice between either producing corresponding garbage output, or performing yet another annoying argument check to ascertain that studentIdList doesn't contain duplicates. But a Map can, by definition, not contain duplicate keys. Problem solved – no garbage input, no additional argument checks.
  
  
  • 
    

    By mapping the student id's to the grades, you could get around the issue of converting between an int and a List<Integer>, which is, unfortunately, a nuisance in Java. Let's assume that you write a method calculateGPA(char) that only calculates the average for one set of grades, i.e. for one student (your instructor's prescribed calculateGPA method could then call this method). Thanks to Java 8, you could then collect the applicable student id's to an int in an elegant way that doesn't require an intermediate List<Integer>:

    
    
    

    public static int getStudentsByGPA(double lower, double higher, Map<Integer, char> studentIdsWithGrades) 
     //argument validation
     return studentIdsWithGrades.entrySet()
     .stream()
     .filter((Map.Entry<Integer, char> studentIdWithGrades) -> 
     double gpa = calculateGPA(studentIdWithGrades.getValue());
     return gpa >= lower && gpa <= higher;
     )
     .mapToInt(Map.Entry::getKey)
     .toArray();
    
    

    
    
  
  

Finally, about the requirement to return null upon encountering invalid arguments and whether "one" would "really do that this way". Generally, one can do whatever one wants. One could also return a single-element array 1000000 when one of the method parameters is invalid if one wants. One should just clearly specify/document the behavior of a method. The more relevant question would probably be what is most useful/most intuitive. Obviously, returning 1000000 is useless and counterintuitive, because this could also be the id of the only student that fulfills the given criterion. Returning null is not ambiguous in this way, but null itself is not really useful either, because it simply doesn't represent anything (or you could say it represents nothing, which, for the purpose of this argument, would amount to the same). I agree with you that an exception would be most useful here, because an exception of an appropriate subclass and with a descriptive message can be self-explanatory (after all, this is what an IllegalArgumentException has been designed for).