Should I validate a method call's return value even if I know that the method can't return bad input?

Question

I'm wondering if I should defend against a method call's return value by validating that they meet my expectations even if I know that the method I'm calling will meet such expectations.

GIVEN

User getUser(Int id)
{
    User temp = new User(id);
    temp.setName("John");

    return temp;
}

SHOULD I DO

void myMethod()
{
    User user = getUser(1234);
    System.out.println(user.getName());
}

OR

void myMethod()
{
    User user = getUser(1234);
    
    // Validating
    Preconditions.checkNotNull(user, "User can not be null.");
    Preconditions.checkNotNull(user.getName(), "User's name can not be null.");

    System.out.println(user.getName());
}

I'm asking this at the conceptual level. If I know the inner workings of the method I'm calling. Either because I wrote it or I inspected it. And the logic of the possible values it returns meet my preconditions. Is it "better" or "more appropriate" to skip the validation, or should I still defend against wrong values before proceeding with the method I'm currently implementing even if it should always pass.

---

My conclusion from all answers (feel free to come to your own):

Assert when

• The method has shown to misbehave in the past
• The method is from an untrusted source
• The method is used from other places, and does not explicitly state it's post-conditions

Do not assert when:

• The method lives closely to yours (see chosen answer for details)
• The method explicitly defines it's contract with something like proper doc, type safety, a unit test, or a post-condition check
• Performance is critical (in which case, a debug mode assert could work as a hybrid approach)

Answer 1

That depends on how likely getUser and myMethod are to change, and more importantly, how likely they are to change independently of each other.

If you somehow know for certain that getUser will never, ever, ever change in the future, then yes it's a waste of time validating it, as much as it is to waste time validating that i has a value of 3 immediately after an i = 3; statement. In reality, you don't know that. But there are some things you do know:

• Do these two functions "live together"? In other words, do they have the same people maintaining them, are they part of the same file, and thus are they likely to stay "in sync" with each other on their own? In this case it's probably overkill to add validation checks, since that merely means more code that has to change (and potentially spawn bugs) every time the two functions change or get refactored into a different number of functions.

• Is the getUser function is part of a documented API with a specific contract, and myMethod merely a client of said API in another codebase? If so, you can read that documentation to find out whether you should be validating return values (or pre-validating input parameters!) or if it really is safe to blindly follow the happy path. If the documentation does not make this clear, ask the maintainer to fix that.

• Finally, if this particular function has suddenly and unexpectedly changed its behavior in the past, in a way that broke your code, you have every right to be paranoid about it. Bugs tend to cluster.

Note that all of the above applies even if you are the original author of both functions. We don't know if these two functions are expected to "live together" for the rest of their lives, or if they'll slowly drift apart into separate modules, or if you have somehow shot yourself in the foot with a bug in older versions of getUser. But you can probably make a pretty decent guess.

Answer 2

Ixrec's answer is good, but I will take a different approach because I believe that it is worth considering. For the purpose of this discussion I will be talking about assertions, since that's the name by which your Preconditions.checkNotNull() has traditionally been known.

What I would like to suggest is that programmers often overestimate the degree by which they are certain that something will behave in a certain way, and underestimate the consequences of it not behaving that way. Also, programmers often do not realize the power of assertions as documentation. Subsequently, in my experience, programmers assert things far less often than they should.

My motto is:

The question you should always be asking yourself is not "should I assert this?" but "is there anything I forgot to assert?"

Naturally, if you are absolutely sure that something behaves in a certain way, you will refrain from asserting that it did in fact behave that way, and that's for the most part reasonable. It is not really possible to write software if you cannot trust anything.

But there are exceptions even to this rule. Curiously enough, to take Ixrec's example, there exists a type of situation where it is perfectly desirable to follow int i = 3; with an assertion that i is indeed within a certain range. This is the situation where i is liable to be altered by someone who is trying various what-if scenarios, and the code which follows relies on i having a value within a certain range, and it is not immediately obvious by quickly looking at the following code what the acceptable range is. So, int i = 3; assert i >= 0 && i < 5; might at first seem nonsensical, but if you think about it, it tells you that you may play with i, and it also tells you the range within which you must stay. An assertion is the best type of documentation, because it is enforced by the machine, so it is a perfect guarantee, and it gets refactored together with the code, so it remains always pertinent.

Your getUser(int id) example is not a very distant conceptual relative of the assign-constant-and-assert-in-range example. You see, by definition, bugs happen when things exhibit behavior which is different from the behavior that we expected. True, we cannot assert everything, so sometimes there will be some debugging. But it is a well established fact in the industry that the quicker we catch an error, the less it costs. The questions you should ask are not only how sure you are that getUser() will never return null, (and never return a user with a null name,) but also, what kinds of bad things will happen with the rest of the code if it does in fact one day return something unexpected, and how easy it is by quickly looking at the rest of the code to know precisely what was expected of getUser().

If the unexpected behavior would cause your database to become corrupt, then maybe you should assert even if you are 101% sure it won't happen. If a null user name would cause some weird cryptic untraceable error thousands of lines further down and billions of clock cycles later, then perhaps it is best to fail as early as possible.

So, even though I do not disagree with Ixrec's answer, I would suggest that you seriously consider the fact that assertions cost nothing, so there is really nothing to be lost, only to be gained, by using them liberally.

Answer 3

A definite no.

A caller should not ever check if the function it is calling respects its contract. The reason is simple: there are potentially very many callers and it is impractical and arguably even wrong from a conceptual point of view for each and every single caller to duplicate the logic for checking the results of other functions.

If any checks are to be done, each function should check its own post-conditions. The post-conditions give you confidence that you implemented the function correctly and the users will be able to rely on the contract of your function.

If a certain function isn't meant to ever return null, then this should be documented and become part of that function's contract. This is the point of these contracts: offer users some invariants that they can rely on so they face fewer hurdles when writing their own functions.

Answer 4

If null is a valid return value of the getUser method, then your code should handle that with an If, not an Exception - it is not an exceptional case.

If null is not a valid return value of the getUser method, then there is a bug in getUser - the getUser method should throw an exception or otherwise be fixed.

If the getUser method is part of an external library or otherwise can't be changed and you want exceptions to be thrown in the case of a null return, wrap the class and method to perform the checks and throw the exception so that every instance of the call to getUser is consistent in your code.

Answer 5

I would argue that the premise of your question is off: why use a null reference to begin with?

The null object pattern is a way to return objects that essentially do nothing: rather than return null for your user, return an object that represents "no user."

This user would be inactive, have a blank name, and other non-null values indicating "nothing here." The primary benefit is you do not need to litter your program will null checks, logging, etc. you just use your objects.

Why should an algorithm care about a null value? The steps should be the same. It is just as easy and far clearer to have a null object that returns zero, blank string, etc.

Here is an example of code checking for null:

User u = getUser();
String displayName = "";
if (u != null) {
  displayName = u.getName();
}
return displayName;

Here is an example of code using a null object:

return getUser().getName();

Which is clearer? I believe the second one is.

---

While the question is tagged java, it is worth noting that the null object pattern is really useful in C++ when using references. Java references are more like C++ pointers, and allow null. C++ references cannot hold nullptr. If one would like to have something analogous to a null reference in C++, the null object pattern is the only way I know of to accomplish that.

Answer 6

Generally, I'd say that depends mainly on the following three aspects:

1. robustness: can the calling method cope with the null value? If a null value might result in a RuntimeException I'd recommend a check - unless complexity is low and called/calling methods are created by the same author, and null is not expected (e.g. both private in the same package).

2. code responsibility: if developer A is responsible for the getUser() method, and developer B uses it (e.g. as part of a library), I'd strongly recommend to validate its value. Just because developer B might not know about a change that results in a potential null return value.

3. complexity: the higher the overall complexity of the program or environment is, the more I'd recommend to validate the return value. Even if you feel sure about that it cannot be null today in the context of the calling method, it might be that you have have to change getUser() for another use case. Once some months or years are gone, and some thousand lines of codes have been added, this can be quite a trap.

In addition, I'd recommend to document potential null return values in a JavaDoc comment. Due to highlighting of JavaDoc descriptions in the most IDEs, this can be a helpful warning for whoever uses getUser().

/** Returns ....
  * @param: id id of the user
  * @return: a User instance related to the id;
  *          null, if no user with identifier id exists
 **/
User getUser(Int id)
{
    ...
}

Answer 7

You definitely don't have to.

For instance, if you already know that a return can never be null - Why would you want to add a null check? Adding a null check is not going to break anything but it's just redundant. And better not code redundant logic as long as you can avoid it.