At a new job, I've been getting flagged in code reviews for code like this:

PowerManager::PowerManager(IMsgSender* msgSender)
  : msgSender_(msgSender) { }

void PowerManager::SignalShutdown()
{
    msgSender_->sendMsg("shutdown()");
}


I'm told that last method should read:

void PowerManager::SignalShutdown()
{
    if (msgSender_) {
        msgSender_->sendMsg("shutdown()");
    }
}


i.e., I must put a NULL guard around the msgSender_ variable, even though it is a private data member. It's difficult for me to restrain myself from using expletives to describe how I feel about this piece of 'wisdom'. When I ask for an explanation, I get a litany of horror stories about how some junior programmer, some-year, got confused about how a class was supposed to work and accidentally deleted a member he shouldn't have (and set it to NULL afterwards, apparently), and things blew up in the field right after a product release, and we've "learned the hard way, trust us" that it's better to just NULL check everything.

To me, this feels like cargo cult programming, plain and simple. A few well-meaning colleagues are earnestly trying to help me 'get it' and see how this will help me write more robust code, but... I can't help feeling like they're the ones who don't get it.

Is it reasonable for a coding standard to require that every single pointer dereferenced in a function be checked for NULL first—even private data members? (Note: To give some context, we make a consumer electronics device, not an air traffic control system or some other 'failure-equals-people-die' product.)

EDIT: In the above example, the msgSender_ collaborator isn't optional. If it's ever NULL, it indicates a bug. The only reason it is passed into the constructor is so PowerManager can be tested with a mock IMsgSender subclass.

SUMMARY: There were some really great answers to this question, thanks everyone. I accepted the one from @aaronps chiefly due to its brevity. There seems to be fairly broad general agreement that:


Mandating NULL guards for every single dereferenced pointer is overkill, but
You can side-step the whole debate by using a reference instead (if possible) or a const pointer, and
assert statements are a more enlightened alternative to NULL guards for verifying that a function's preconditions are met. 


It depends on the 'contract':

If PowerManager MUST have a valid IMsgSender, never check for null, let it die sooner.

If on the other hand, it MAY have a IMsgSender, then you need to check every time you use, as simple as that.

Final comment about the story of the junior programmer, the problem is actually the lack of testing procedures.
