I have to make this student repository where exceptions need to be thrwon if certain conditions are not met. My question is: should I create my own custom exceptions, or should I use the predefined ones? I will leave a portion of my code below

public class StudentRepository {

private static List<Student> studentList = new ArrayList<>();
private static Set<Student> studentsByAge = new TreeSet<>(new AgeComparator());
private static Set<Student> studentsByLastName = new TreeSet<>(new LastNameComparator());

public static Student addStudent(String firstName, String lastName, String gender, LocalDate dateOfBirth) {


    if (firstName.trim().isEmpty() || lastName.trim().isEmpty()) {
        throw new NullPointerException("Last name or first name field may be empty");
    } else if (!(gender.equalsIgnoreCase("m") || gender.equalsIgnoreCase("f"))) {
        throw new IllegalArgumentException("Gender should be 'M or 'F");
    } else if (dateOfBirth.isBefore(LocalDate.of(1900, 1, 1)) ||
            dateOfBirth.isAfter(LocalDate.now())) {
        throw new IllegalArgumentException("Date of birth should be between 1900 and current year");
    }

    Student newStudent = new Student(firstName, lastName, dateOfBirth, gender);
    studentList.add(newStudent);
    return newStudent;
}

public static void deleteStudentByCnp(String cnp) {
    Student studentToDelete = null;

    for (Student student : studentList) {
        if (student.getCnp().equals(cnp)) {
            studentToDelete = student;
            studentList.remove(studentToDelete);
            break;
        } else {
            throw new NullPointerException("The student does not exist");
        }
    }
}

CodePudding user response：

You can check Oracle's recommendations on checked vs unchecked exceptions topic which should make it more clear: https://docs.oracle.com/javase/tutorial/essential/exceptions/runtime.html

and the summary from the page is the following

Here's the bottom line guideline: If a client can reasonably be expected to recover from an exception, make it a checked exception. If a client cannot do anything to recover from the exception, make it an unchecked exception.

You can also read more about usage of custom exceptions vs standard ones here: https://stackify.com/java-custom-exceptions/

Generally it depends on client behaviour - what client can do with this exception. If it's only logging error and nothing more - then IllegalArgumentException should be enough but if you then return this error to client, e.g. using REST - then it would make sense to create custom exception with proper structure that would be serialized and returned in response to keep consistent structure of errors in response.

Anyway, please do not use NullPointerException besides cases when something is null unexpectedly (i.e. it's programmer's error). In your case I would replace NullPointerExceptions with custom exception.

CodePudding user response：

Assuming these rules (gender must be M or F, etc) are considered preconditions (in that the docs call out: The params you pass must adhere to these rules), the correct exception to throw is IllegalArgumentException. Except for a precondition that a param must not be null, in which case NullPointerException is appropriate. Talking about recoverability is not appropriate for that kind of exception - the author of a method has no idea if the caller is the kind of code that can 'reasonably recover from the exception'. There's a decent chance they can't (say, it's text provided over a network and there's no mechanism to signal back to the caller: Err, you're not following the agreed upon spec there, or it's from a config file), so unchecked is probably better here. IllegalArgumentException just fits.

NullPointerException, though, is clearly really bad usage here. That is to signal a thing is null that wasn't expected to be, and that is all, so don't abuse it like this!

Note that your code has plenty of problems:

• 2 genders? It's 2021. This probably was never acceptable but certainly isn't now. Why do you need to know the gender of a student? To put the right honorific on top of a letter? Get rid of gender and add a field for 'Honorific' instead. Various countries have guidelines against asking for gender when it is not relevant. I doesn't seem relevant here. (Honorific might be; if you need that, ask for that).
• If you want ignore that and force 2 genders, then force it, and write an enum instead. You want to avoid the entire idea that some inputs can be stated but are invalid - because then you need to keep writing code to react to invalid state. Much better if an object or param simply cannot be wrong. An enum cannot be wrong, or at least, can only be wrong if e.g. it is null, which is a simpler check.
• Your 'date' error says 'current year' but actually checks current day.
• The error for gender has typos.
• Your message for lastname/firstname is unclear and doesn't follow java styles. You should state the rule, not the problem. 'First and Last name must not be empty'. There's nothing 'may' about it.
• Your delete method will check the first student in the list. If its CNP matches, that is deleted, and you're done. If it does not match, your code errors out immediately. It never scans any student other than the very first in the list. That exception throw needs to be outside the for loop.
• a Map sounds like a better datastructure here; currently your delete code will take longer and longer as there are more and more students.
• While you're making the code more general, the concept of 'first' and 'last' name is dubious as well. This is good news: It simplifies our lives as developers. Just have a 'full name' field. Now you no longer have to do the math on how to combine first and last name together. You just print the value of the 'full name' field. If you have a need for a shorter name, then add that as a field instead. Same for gender: Instead of figuring out the right honorific based on gender, just put the value of the 'honorific' field up there.