30

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)
  • 1
    Aim for that %100 code coverage! – Jared Burrows Apr 29 '15 at 3:13
  • 9
    Why are you focusing on just one problem (Null)? It's probably equally problematic if the name is "" (not null but the empty string) or "N/A". Either you can trust the result, or you have to be appropriately paranoid. – MSalters Apr 29 '15 at 9:32
  • 3
    In our codebase we have a naming scheme: getFoo() promises not to return null, and getFooIfPresent() may return null. – pjc50 Apr 29 '15 at 17:00
  • Where does your presumption of code sanity come from? Are you assuming the value will always be non-null because it's validated on entry, or because of the structure by which the value is retrieved? And, more importantly, are you testing every possible edge case scenario? Including the possibility of malicious script? – Zibbobz Apr 29 '15 at 18:25
  • @MSalters Don't validate the value if it is there ;) – Izkata Apr 29 '15 at 18:29
42

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.

  • 2
    Also: If you were to change getUser later, so that it could return null, would the explicit check give you information you couldn't get from "NullPointerException" and the line number? – immibis Apr 29 '15 at 4:26
  • It seems there's two things you can validate: (1) That the method works properly, as in, it has no bugs. (2) That the method respects your contract. I can see validating for #1 to be overkill. You should have tests doing #1 instead. Which is why I wouldn't validate i = 3;. So my question relates to #2. I like your answer for that, but at the same time, it's a use your judgment answer. Do you see any other problem arising from explicitly validating your contracts other then wasting a bit of time writing the assert? – Didier A. May 5 '15 at 17:24
  • The only other "problem" is that if your interpretation of the contract is wrong, or becomes wrong, then you have to go change all the validation. But that applies to all other code too. – Ixrec May 5 '15 at 18:01
  • 1
    Can't go against the crowd. This answer is definitely the most correct in covering the topic of my question. I'd like to add that from reading all the answers, especially @MikeNakis, I now think you should assert your pre-conditions, at least in debug mode, when you have either of the last two bullet points of Ixrec. If you are faced with the first bullet point, then it's probably overkill to assert your preconditions. Finally, given an explicit contract, like proper doc, type safety, a unit test, or a post-condition check, you shouldn't assert your preconditions, that would be overkill. – Didier A. May 7 '15 at 19:03
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 , 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.

  • Null is just the example of what I assert. It could very well be I need a non empty name. My question would still apply, would you validate that? Or it could be something that returns an int, and I can't have a negative int, or it must be in a given range. Don't think of it as a null problem per say. – Didier A. Apr 28 '15 at 22:58
  • 1
    Why would a method return an incorrect value? The only place this should be checked is at the boundary of your application: interfaces with the user and with other systems. Otherwise, that is why we have exceptions. – user22815 Apr 28 '15 at 23:00
  • Imagine I had: int i = getCount; float x = 100 / i; If I know getCount does not return 0, because I either wrote the method or inspected it, should I skip the check for 0? – Didier A. Apr 29 '15 at 0:00
  • @didibus: you can skip the check if getCount() documents a guarantee that it does not now return 0, and will not do so in future except in circumstances where someone decides to make a breaking change and goes looking to update everything that calls it to cope with the new interface. Seeing what the code currently does is no help, but if you're going to inspect code then the code to inspect is the unit tests for getCount(). If they test it not to return 0, then you know any future change to return 0 will be considered a breaking change because the tests will fail. – Steve Jessop Apr 29 '15 at 0:06
  • It is not clear to me that null objects are better than nulls. A null duck may quack, but it is not a duck - in fact, semantically, it is the antithesis of a duck. Once you introduce a null object, any code using that class may have to check if any of its objects are the null object - e.g. you have a null something in a set, how many things do you have? This looks to me like a way to defer failure and obfuscate errors. The linked article specifically warns against using null objects "just to avoid null checks and make code more readable". – sdenham Apr 29 '15 at 20:16
22

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.

  • 2
    Yes. Assert it. I came here to give more or less this answer. ++ – RubberDuck Apr 29 '15 at 0:02
  • 2
    @SteveJessop I would correct this to ... && sureness < 1). – Mike Nakis Apr 29 '15 at 0:22
  • 2
    @MikeNakis: that's always a nightmare when writing unit tests, the return value that's permitted by the interface but impossible to actually generate from any inputs ;-) Anyway, when you're 101% sure you're right then you're definitely wrong! – Steve Jessop Apr 29 '15 at 0:26
  • 2
    +1 for pointing out things I completely forgot to mention in my answer. – Ixrec Apr 29 '15 at 6:34
  • 1
    @DavidZ That's correct, my question assumed run-time validation. But saying you don't trust the method in debug and trust it in prod is a possibly good view point on the topic. So a C style assert might be a good way to deal with this situation. – Didier A. Apr 29 '15 at 18:51
-1

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.

  • 1
    I know, but only because I inspected or wrote the other method's implementation. The method's contract does not explicitly say that it can't. So it could, for example, change in the future. Or it could have a bug which causes a null to return even if it was trying not to. – Didier A. May 5 '15 at 17:34
8

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.

  • I agree with the general sentiment, but there are circumstances where it definitely makes sense to validate return values. For example if your calling an external Web Service, you sometimes want to at least validate the format of the answer (xsd, etc...) – AK_ Apr 30 '15 at 9:25
  • Seems like you're advocating a Design by Contract style over a Defensive Programming one. I think this is a great answer. If the method had a strict contract, I could trust it. In my case it does not, but I could change it to do so. Say I couldn't though? Would you argue that I should trust the implementation? Even if it does not explicitly state in it's doc that it does not return null, or actually has a post-condition check? – Didier A. May 5 '15 at 17:30
  • In my opinion, you should use your best judgement and weight different aspects like how much you trust the author to do the right thing, how likely is the function's interface to change, and how tight your time constraints are. That being said, most of the time, the author of a function has a pretty clear idea of what the function's contract should be, even if he does not document it. Because of this, I say you should first trust authors to do the right thing and only become defensive when you know that the function is not well written. – Paul May 6 '15 at 10:59
  • I find that first trusting others to do the right thing helps me get the job done faster. That being said, the kind of applications I write are easy to fix when a problem does occur. Someone working with more safety-critical software might think that I'm being too lenient and prefer a more defensive approach. – Paul May 6 '15 at 11:05
5

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.

  • Null is not a valid return value of getUser. By inspecting getUser, I saw that the implementation would never return null. My question is, should I trust my inspection and not have a check in my method, or should I doubt my inspection, and still have a check. It's also possible the method changes in the future, breaking my expectations of not returning null. – Didier A. Apr 29 '15 at 0:06
  • And what if getUser or user.getName change to throw exceptions? You should have a check, but not as part the method using the User. Wrap the getUser method - and even the User class - to enforce your contracts, if you're worried about future changes to the getUser code breaking. Also create unit tests to check your assumptions about the behaviour of the class. – MatthewC Apr 29 '15 at 0:16
  • @didibus To paraphrase your answer... A smiley emoticon is not a valid return value of getUser. By inspecting getUser, I saw that the implementation would never return a smiley emoticon. My question is, should I trust my inspection and not have a check in my method, or should I doubt my inspection, and still have a check. It's also possible the method changes in the future, breaking my expectations of not returning a smiley emoticon. It is not the job of the caller to confirm that the called function is fulfilling its contract. – Vince O'Sullivan Apr 29 '15 at 11:39
  • @VinceO'Sullivan It seems like the "contract" is at the center of the issue. And that my question is about implicit contracts, versus explicit. A method that returns int I wont assert that I'm not getting a string. But, if I only want positive ints, should I? It's only implicit that the method does not return negative int, because I inspected it. It's not clear from the signature or the documentation that it does not. – Didier A. May 5 '15 at 19:14
  • 1
    It doesn't matter if the contract is implicit or explicit. It is not the job of the calling code to validate that the called method returns valid data when given valid data. In you're example, you know that negative inits are not incorrect answers but do you know if positive int are correct? How many tests do you really need, to prove that the method is working? The only correct course is to assume that the method is correct. – Vince O'Sullivan May 6 '15 at 15:50
1

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)
{
    ...
}

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.