If functions have to do null checks before doing the intended behaviour is this bad design?

Question

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)?

whatsisname · Accepted Answer · 2018-07-04T21:01:12.490

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:

Don't bother checking, and just let the runtime throw the nullpointer exception.
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:

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.

David Arno · Answer 2 · 2018-07-03T17:23:18.423

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:

make a local copy of _someEntity and reference that copy through the method,
use a lock around _someEntity to ensure it doesn't change as the method executes,
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.

score 23 · Answer 3 · edited Jul 05 '18 at 21:39

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?

score 14 · Answer 4 · answered Jul 03 '18 at 19:35

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.

score 8 · Answer 5 · answered Jul 03 '18 at 14:39

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.

score 4 · Answer 6 · answered Jul 03 '18 at 12:06

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.

score 4 · Answer 7 · answered Jul 03 '18 at 20:48

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.

If functions have to do null checks before doing the intended behaviour is this bad design?

7 Answers7