Should I always check member pointers for nullptr?

Question

Is it bad to do something like this? (not checking for nullptr inside the draw() function before doing operations with the object pointer)

class SomeClass
{
public:
    SomeClass(Object& someValidObject)
    {
       object = &someValidObject;
    }

    void draw()
    {
        object->draw();
    }

    Object* object = nullptr;
}

Or should I always check for nullptr before calling operations in the object, even though the constructor is for sure going to make the pointer point to something?

void draw()
{
   if(object != nullptr) 
      object->draw?
}

Answer 1

Depends what you are trying to protect against.

As was pointed out in the comments already, you cannot reliably check for dangling pointers, so if the passed Object goes out of scope before your SomeClass, bad things will happen.

So the only thing that you can reliably check is whether the pointer is nullptr, but as you have noticed yourself, currently the constructor makes that next to impossible. However, as long as your object member variable is public like it is now, the user could in theory reach in there and set it to nullptr. You can make that harder by making the member private and use a setter which rejects nullptr values (or simply takes a reference, like the constructor). In such a design, object != nullptr can be considered a class invariant, that is a condition that is true before and after every member function call (from the time after construction until before destruction).

So how do we break a class invariant? As a client, we could violate a function's preconditions and thus bring the class into an undefined state. With a class as simple as your sample, this is hard to do, as the functions do not really have preconditions. But let's assume for arguments sake, you were to add a setter like this:

// Precondition: obj must point to a valid Object.
// That object must be kept alive for the lifetime of the class,
// otherwise the behavior is undefined.
void setObject(Object* obj)
{
    object = obj;
}

Now this is somewhat subtle. The code allows us to pass a nullptr here, but the documentation explicitly forbids it. If we pass a nullptr or have the passed object die before our SomeClass, we violate the contract of that class. But this contract is not enforced in code, it's only in the comments.

The important thing to realize here is that there are conditions that cannot be checked in code. We can check for nullptr, but we cannot check for dangling pointers. Sometimes checks would be possible, but are undesirable due to high runtime costs (eg. check that a range is sorted for a binary search). Once we realize this it becomes clear that we have some wiggle room here as a class designer. Since we anyway cannot make things 100% bullet-proof, should we check anything at all? Sure, we can check for nullptr everywhere, but that means paying the runtime overhead for checking what is basically a programming error.

What if I don't want to pay this overhead in production builds? Maybe I would like my debugger to catch it if I make a mistake during development, but I don't want my clients to have to pay for the check later after release.

So what you are really looking for is an assert:

void draw()
{
    assert(object);
    object->draw();
}

The decision whether to check something in an assert (or not at all) or have a proper runtime check that is part of the contract is not an easy one. It is more often than not a philosophical question. John Lakos gave an excellent talk about this at CppCon 2014 if you want to dig deeper.

Answer 2

I guess it is a matter of taste. But I think it is good practice for the sake of maintainability to express expectations inside the code (and as code); therefore I would rather write:

void draw()
{
   if(object == nullptr) { /* throw some exception */ }

   object->draw()
}

Edit

As suggested in the comments of my answer it is usually not enough to only check for null pointer in C++. Indeed, a pointer can be non-null yet invalid (this is called a dangling pointer). This happens when object is destroyed before the draw function is called.