66

So I don't know if this is good or bad code design so I thought I better ask.

I frequently create methods that do data processing involving classes and I often do a lot of checks in the methods to make sure I don't get null references or other errors before hand.

For a very basic example:

// fields and properties
private Entity _someEntity;
public Entity SomeEntity => _someEntity;

public void AssignEntity(Entity entity){
    _someEntity = entity;
}
public void SetName(string name)
{
    if (_someEntity == null) return; //check to avoid null ref
    _someEntity.Name = name;
    label.SetText(_someEntity.Name);
}

So as you can see im checking for null every time. But should the method not have this check?

For example should external code cleanse the data before hand so the methods don't need to validate like below:

 if(entity != null) // this makes the null checks redundant in the methods
 {
     Manager.AssignEntity(entity);
     Manager.SetName("Test");
 }

In summary, should methods be "data validating" and then do their processing on the data, or should it be guaranteed before you call the method, and if you fail to validate prior to calling the method it should throw an error (or catch the error)?

  • 7
    What happens when someone fails to do the null check and SetName throws an exception anyway? – Robert Harvey Jul 2 '18 at 20:58
  • 4
    What happens if I actually want someEntity to be Null? You decide what you want, either someEntity can be Null, or it can't, and then you write your code accordingly. If you want it never to be Null, you can replace checks with asserts. – gnasher729 Jul 2 '18 at 21:04
  • Note that some languages do have the option to make this parameter contract explicit, e.g. Scala’s Option<Entity>. While you could still pass a null Option, that would clearly be a mistake of the caller. But an empty Option would be fine. – Sebastiaan van den Broek Jul 3 '18 at 3:37
  • 5
    You can watch Gary Bernhardt's Boundaries talk about making a clear division between sanitizing data (ie, checking for nulls, etc) in an outer shell, and having an inner core system that doesn't have to care about that stuff - and just do its work. – mgarciaisaia Jul 3 '18 at 12:12
  • 3
    This is one of the reasons why the C# language team wants to introduce nullable reference types (and non nullable ones) -> github.com/dotnet/csharplang/issues/36 – Mafii Jul 4 '18 at 7:00
167

The problem with your basic example isn't the null check, it's the silent fail.

Null pointer/reference errors, more often than not, are programmer errors. Programmer errors are often best dealt with by failing immediately and loudly. You have three general ways to deal with the problem in this situation:

  1. Don't bother checking, and just let the runtime throw the nullpointer exception.
  2. Check, and throw an exception with a message better than the basic NPE message.

A third solution is more work but is much more robust:

  1. Structure your class such that it's virtually impossible for _someEntity to ever be in an invalid state. One way to do that is to get rid of AssignEntity and require it as a parameter for instantiation. Other Dependency Injection techniques can be useful as well.

In some situations, it makes sense to check all function arguments for validity before whatever work you're doing, and in others it make sense to tell the caller they are responsible for ensuring their inputs are valid, and not checking. Which end of the spectrum you're on is going to depend on your problem domain. The third option has a significant benefit in that you can, to an extent, have the compiler enforce that the caller does everything properly.

If the third option is not an option, then in my opinion it really doesn't matter as long as it doesn't silently fail. If not checking for null causes the program to instantly explode, fine, but if it instead quietly corrupts the data to cause trouble down the road, then it's best to check and deal with it right then and there.

  • 10
    @aroth: Any design of anything, software included, will come with constraints. The very act of designing is selecting where those constraints go, and it is not my responsibility nor should it be that I accept any input and be expected to do something useful with it. I do know whether null is wrong or invalid because in my hypothetical library I've defined the data types and I've defined what is valid and what isn't. You call that "forcing assumptions" but guess what: that's what every software library in existence does. There are times where null is a valid value, but that's not "always". – whatsisname Jul 4 '18 at 4:26
  • 4
    "that your code is broken because it doesn't handle null properly" - This is a misunderstanding of what proper handling means. For most Java programmers, it means throwing an exception for any invalid input. Using @ParametersAreNonnullByDefault, it means also for every null, unless the argument is marked as @Nullable. Doing anything else means lengthening the path until it finally blows elsewhere and thereby also lengthening the time wasted on debugging. Well, by ignoring problems, you may achieve that it never blows, but incorrect behavior is then pretty guaranteed. – maaartinus Jul 4 '18 at 5:17
  • 4
    @aroth Dear Lord, I would hate to work with any code you write. Explosions are infinitely better than doing something nonsensical, which is the only other option for invalid input. Exceptions force me, the caller, decide what the right thing to do is in those cases, when the method cannot possibly guess what I intended. Checked exceptions and unnecessarily extending Exception are completely unrelated debates. (I agree both are troublesome.) For this specific example, null obviously makes no sense here if the purpose of the class is to invoke methods on the object. – jpmc26 Jul 4 '18 at 17:26
  • 3
    "your code shouldn't be forcing assumptions out into the caller's world" - It's impossible to not force assumptions out to the caller. That's why we need to make these assumptions as explicit as possible. – Diogo Kollross Jul 4 '18 at 18:12
  • 3
    @aroth your code is broken because its passing my code a null when it should be passing something that has Name as a property. Any behaviour other than exploding is some kind of backdoor bizarro-world version of dynamic typing IMHO. – mbrig Jul 4 '18 at 20:02
55

Since _someEntity can be modified at any stage, then it makes sense to test it every time that SetName is called. After all, it could have changed since the last time that method was called. But be aware that the code in SetName isn't thread-safe, so you can perform that check in one thread, have _someEntity set to null by another and then the code will barf a NullReferenceException anyway.

One approach to this problem therefore is to get even more defensive and do one of the following:

  1. make a local copy of _someEntity and reference that copy through the method,
  2. use a lock around _someEntity to ensure it doesn't change as the method executes,
  3. use a try/catch and perform the same action on any NullReferenceException's that occur within the method as you'd perform in the initial null check (in this case, a nop action).

But what you should really do is stop and ask yourself the question: do you actually need to allow _someEntity to be overwritten? Why not set it once, via the constructor. That way, you need only ever need do that null check the once:

private readonly Entity _someEntity;

public Constructor(Entity someEntity)
{
    if (someEntity == null) throw new ArgumentNullException(nameof(someEntity));
    _someEntity = someEntity;
}

public void SetName(string name)
{
    _someEntity.Name = name;
    label.SetText(_someEntity.Name);
}

If at all possible, this is the approach I'd recommend taking in such situations. If you can't be mindful of thread safety when choosing how to handle possible nulls.

As a bonus comment, I feel your code is strange and could be improved. All of:

private Entity _someEntity;
public Entity SomeEntity => _someEntity;

public void AssignEntity(Entity entity){
    _someEntity = entity;
}

can be replaced with:

public Entity SomeEntity { get; set; }

Which has the same functionality, save for being able to set the field via the property setter, rather than a method with a different name.

  • 5
    Why does this answer the question? The question addresses null checking and this is pretty much a code review. – IEatBagels Jul 3 '18 at 17:05
  • 17
    @TopinFrassi it explains that the current null checking approach is not thread safe. It then touches on other defensive programming techniques to get around that. Then it wraps up with the suggestion of using immutability to avoid the need to have that defensive programming in the first place. And as an added bonus, it offers advice on how to structure the code better too. – David Arno Jul 3 '18 at 17:09
23

But should the method not have this check?

This is your choice.

By creating a public method, you are offering the public the opportunity to call it. This always comes along with an implicit contract on how to call that method and what to expect when doing so. This contract may (or may not) include "yeah, uhhm, if you pass null as a parameter value, it will explode right in your face.". Other parts of that contract might be "oh, BTW: every time two threads execute this method at the same time, a kitten dies".

What type of contract you want to design is up to you. The important things are to

  • create an API that's useful and consistent and

  • to document it well, especially for those edge cases.

What are the likely cases how your method is being called?

  • Well, thread-safety is the exception, not the rule, when there is concurrent access. Thus use of hidden / global state is documented, as well as any case concurrency is safe anyway. – Deduplicator Jul 5 '18 at 21:56
14

The other answers are good; I'd like to extend them by making some comments about accessibility modifiers. First, what to do if null is never valid:

  • public and protected methods are those where you do not control the caller. They should throw when passed null. That way you train your callers to never pass you a null, because they would like their program to not crash.

  • internal and private methods are those where you do control the caller. They should assert when passed null; they will then probably crash later, but at least you got the assertion first, and an opportunity to break into the debugger. Again, you want to train your callers to call you correctly by making it hurt when they do not.

Now, what do you do if it might be valid for a null to be passed in? I would avoid this situation with the null object pattern. That is: make a special instance of the type and require that it be used any time that an invalid object is needed. Now you are back in the pleasant world of throwing/asserting on every use of null.

For example, you don't want to be in this situation:

class Person { ... }
...
class ExpenseReport { 
  public void SubmitReport(Person approver) { 
    if (approver == null) ... do something ...

and at the call site:

// I guess this works but I don't know what it means
expenses.SubmitReport(null); 

Because (1) you're training your callers that null is a good value, and (2) it is unclear what the semantics of that value are. Rather, do this:

class ExpenseReport { 
  public static readonly Person ApproverNotRequired = new Person();
  public static readonly Person ApproverUnknown = new Person();
  ...
  public void SubmitReport(Person approver) { 
    if (approver == null) throw ...
    if (approver == ApproverNotRequired) ... do something ...
    if (approver == ApproverUnknown) ... do something ...

And now the call site looks like this:

expenses.SubmitReport(ExpenseReport.ApproverNotRequired);

and now the reader of the code knows what it means.

  • Better yet, don’t have a chain of ifs for the null objects but rather use polymorphism to make them behave correctly. – Konrad Rudolph Jul 4 '18 at 10:29
  • @KonradRudolph: Indeed, that's often a good technique. I like to use that for data structures, where I make an EmptyQueue type that throws when dequeued, and so on. – Eric Lippert Jul 4 '18 at 17:37
  • @EricLippert - Forgive me because I'm still learning here, but suppose if the Person class was immutable, and it didn't contain a zero argument constructor, how would you construct your ApproverNotRequired or ApproverUnknown instances? In other words, what values would you pass the constructor? – user306112 Jul 5 '18 at 0:54
  • 2
    I love bumping into your answers/comments across SE, they're always insightful. As soon as I saw asserting for internal/private methods I scrolled down expecting it to be an Eric Lippert answer, wasn't disappointed :) – bob esponja Jul 5 '18 at 10:46
  • 4
    You guys are over-thinking the specific example I gave. It was intended to get the idea of the null object pattern across quickly. It was not intended to be an example of good design in a papers-and-paychecks automation system. In a real system, making "approver" have type "person" is probably a bad idea because it does not match the business process; there might be no approver, you might need several, and so on. As I often note when answering questions in that domain, what we really want is a policy object. Don't get hung up on the example. – Eric Lippert Jul 5 '18 at 13:37
8

The other answers point out that your code can be cleaned up to not need a null check where you have it, however, for a general answer on what a useful null check can be for consider the following sample code:

public static class Foo {
    public static void Frob(Bar a, Bar b) {
        if (a == null) throw new ArgumentNullException(nameof(a));
        if (b == null) throw new ArgumentNullException(nameof(b));

        a.Something = b.Something;
    }
}

This gives you an advantage for maintenance. If there ever occurs an defect in your code where something passes a null object to the method you will get useful information from method as to which parameter was null. Without the checks, you will get a NullReferenceException on the line:

a.Something = b.Something;

And (given that the Something property is a value type) either a or b could be null and you will have no way of knowing which from the stack trace. With the checks, you will know exactly which object was null, which can be enormously useful when trying to unpick a defect.

I would note that the pattern in your original code:

if (x == null) return;

Can have its uses in certain circumstances, it can also (possibly more often) give you a very good way of masking underlying issues in your codebase.

4

No not at all, but it depends.

You only do a null reference check if you want to react on it.

If you do a null reference check and throw a checked exception, you force the user of the method to react on it and allow him to recover. The program still works as expected.

If you don't do a null reference check (or throw a unchecked exception), it might throw a unchecked NullReferenceException, which usually is not handled by the user of the method and even might shut down the application. This means that the function is obviously completely misunderstood and therefore the application is faulty.

4

Something of an extension to @null's answer, but in my experience, do null checks no matter what.

Good defensive programming is the attitude that you code the current routine in isolation, on the assumption that the entire rest of the program is trying to crash it. When you're writing mission critical code where failure is not an option, this is pretty much the only way to go.

Now, how you actually deal with a null value, that depends a great deal on your use case.

If null is an acceptable value, e.g. any of the second through fifth parameters to the MSVC routine _splitpath(), then silently dealing with it is the correct behavior.

If null should not ever happen when calling the routine in question, i.e. in the OP's case the API contract requires AssignEntity() to have been called with a valid Entity then throw an exception, or otherwise fail ensuring you make a lot of noise in the process.

Never worry about the cost of a null check, they're dirt cheap if they do happen, and with modern compilers, static code analysis can and will elide them completely if the compiler determines that it's not going to happen.

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.