Should I write null checks for all of my methods?

Question

I'm creating my own library like Apache Commons DigestUtils for learning purposes.

Inside of my update() method I've written a simple if (something == null) throw new Exception as null checking.

/**
 * Performs a digest update for the {@code String} (converted to bytes using the
 * UTF-8 standard charsets) with a specific {@code MessageDigest} and returns
 * the final digest data.
 *
 *
 * @param messageDigest the {@code MessageDigest} with a specific algorithm to
 *                      process the digest.
 *
 * @param data          the data to digest.
 *
 * @return the {@code MessageDigest} with the processed update for the digest.
 *
 * @throws IllegalArgumentException if {@code messageDigest} is {@code null}.
 *
 * @throws IllegalArgumentException if {@code data} is {@code null}.
 */
public static MessageDigest update(final MessageDigest messageDigest, final String data) {
    if (messageDigest == null)
        throw new IllegalArgumentException("messageDigest cannot be null");
    if (data == null)
        throw new IllegalArgumentException("data cannot be null");

    messageDigest.update(data.getBytes(StandardCharsets.UTF_8));

    return messageDigest;
}

Now I'm not sure if I should write the same checks in other methods that must call the update() method, or only write it in the comments that the method throws the corresponding exceptions.

public static byte[] digest(final MessageDigest messageDigest, final byte[] data) {
    return update(messageDigest, data).digest();
}

or

public static byte[] digest(final MessageDigest messageDigest, final byte[] data) {
    if (messageDigest == null)
        throw new IllegalArgumentException("messageDigest cannot be null");
    if (data == null)
        throw new IllegalArgumentException("data cannot be null");

    return update(messageDigest, data).digest();
}

Note that digest() method calls update() method.

What is the best practice?

Answer 1

1.: There is the function Objects.requireNonNull, which is exactly for this purpose. It throws an exception if the given argument equals null.

if (messageDigest == null)
        throw new IllegalArgumentException("messageDigest cannot be null");

should be just

Objects.requireNonNull(messageDigest, "messageDigest cannot be null");

2.: The best practice to throw an exception because of null is to throw it where the null shouldn't occur in the first place. This is the motivation behind the Fail Fast Principle.

When a null reference occurs where it should not be, you want to throw an exception, or handle it, at exactly that spot. You don't want to pass the null referece to other methods/functions. Otherwise you would have a harder time debugging your code when the passed null reference causes problems in your program. This is because it won't be clear where the null reference occured in the first place; but what actually caused it is what you want to fix/handle. An example:

public class Demo {
    public void a(Object obj) {
        b(obj);
    }
    private void b(Object obj) {
        c(obj);
    }
    private void c(Object obj) {
        // Calculation using obj.
    }
}

If it's clear that a should never get a null reference passed, this is the correct place to check for it.

public void a(Object obj) {
    Objects.requireNonNull(obj); // Throw exception when null.
    b(obj);
}

It's bad when you do a null check in every single method because the fact that obj shouldn't be null counts for all those methods, thus it's enough to place the null check where the null shouldn't occur in the first place, i.e. a. The program shall fail fast.

It would be even worse to put the null check in c because c should logically never receive a null reference from b, and b shouldn't receive one from a. If the null reference causes problems in c, you will have a harder time finding out if null occured in b or a or the code that called a. You want to know that because you want to fix the problem of the occuring null reference, not the problems caused by it. Of course, sometimes you can't fix the occurrence itself, but you can try to get as close as possible to the occurrence and reduce collateral "damage" done by it.

There is no real goto way to place null checks. You have to place them at the right places to make the program fail fast on occurring invalid objects. It's up to you to decide where the best places for null checks are because it all depends on the exact case.

I hope this gives you a good idea about how to approach this.

Answer 2

The better thing to do is just let the update() method do it's thing, and if it throws an Exception, you throw an exception too. There are two common things to do, both throw the Exception that you get one too. One way to put a throws SomeException in the method name where SomeException is the Exception that you are excepting.

public static byte[] digest(final MessageDigest messageDigest, final byte[] data) throws IllegalArgumentException {
    return update(messageDigest, data).digest();
}

So if you get an Exception from update(), it will automatically throw it. NOTE: you don't need to throw IllegalArgumentException's because they are a type of RunTimeException, which are automatically thrown. The second way is try catch, which gives you a little more options.

public static byte[] digest(final MessageDigest messageDigest, final byte[] data) throws IllegalArgumentException {
    try {
        return update(messageDigest, data).digest();
    } catch(IllegalArgumentException e) {
        // Do whatever you want. You can even throw e if you want.
    }
}

This allows you to do whatever you want if you find there was an exception.

Answer 3

1. Starting with JDK15 (or is it in 14 already?), NPEs thrown by the system itself (i.e. when you run, say, Object o = null; o.hashCode(); will have all the details; in particular the text of the expression. That means that if in your code an NPE would occur 'naturally' (because you dereference the variable, or pass it along to another method that always does that), there's no need for the null check. Stylistically the problem with that is if later you change your code so that it doesn't necessarily dereference, you lose the effect that invoking the method will a null argument throws. Something to keep in mind.

2. It is in my experience more common to explicitly throw 'NullPointerException', i.e. to write if (param == null) throw new NullPointerException("param"). The various libraries that automate throwing this also default to NPE as a rule.

3. You do not need to write the if. java core ships with a utility to one-liner it: Objects.requireNonNull(param, "param"); is a lot shorter than your 3-line if block!

4. You can use lombok's @NonNull to generate these null checks automatically, saves you even more typing.

5. You can configure your IDE to warn on changing parameter values; this means you no longer have to put the keyword 'final' 15,950,951 (approximation) times in your source files. Saves you a lot of typing.

6. Whilst the other answer in this question so far says that you don't need to declare throws IllegalArgumentException, which is true, doing it for documentation purposes is (again, my experience) common.

7. There are annotations to mark non-nullity. Aside from the lombok one, various libraries exist that give you compile-time null checking, and they generally are built into the major IDEs, or there's a plugin for them, so you get as-you-write errors that you're passing (potential) null values to methods that don't want it, or doing useless null checks (null checks on for example values returned from methods that indicate they never return null). If you can rely on all the users of a library to use such tools, you don't need the runtime nullchecks at all, but I would suggest you keep them; you can't rely on your users to have these tools, I think. Nevertheless, consider annotating your params so that those who do get the benefit of the compile-time checks.

For the compile-time annotations, you have a wide array of choices here (unfortunately): There's checkerframework which is by far the most expansive, letting you go a lot further than compile-time null check guards. There's also the baked-into-your-IDE sets: eclipse's and intellij's are the most common, but there are many more.

Only lombok's will generate a runtime nullcheck for you, and IDEs can be configured to consider it for compile-time analysis as well, giving you the best of both worlds. Alternatively, combine one of these framework's annotations with Objects.requireNonNull.