C++ Standards – Is It Reasonable to Null Guard Every Dereferenced Pointer

ccoding-standards

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.

Best Answer

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.

Related Solutions

Should I strictly follow every single HTML and CSS Standard

Product first, then polish.

Get your site/application/game doing what it's supposed to do. Get it up and running, and get people interested.

Then, when you have the time, go back and polish it up. But only because you care, not because anybody else does.

Of course, if the non-compliance issues mean people can't view it, or it's unreadably ugly, or it takes a month to load, or it's hard to maintain, or it crashes the browser, this is a major problem. But it would still be a major problem even if you were standards-compliant.

Ordinary users do not look at the source for a website that isn't loading and go, "Well, it's not displaying the pictures, but it's completely W3C-compliant". They simply browse to another website and never return.

Bottom line, standards are there to make writing browsers easier, and to close up potential security holes. Amazon, Penny-Arcade and Stack Overflow do not make their money from running a standard-compliant website. And unless you're in a website-writing competition, neither will you.

Object-oriented – Filesystem like permissions for C++ type-members

Rephrasing the problem a bit:

I have an instance of MySpecialObject
an instance of JLP wants to call the methods of MySpecialObject
an instance of BEV wants to read the public data of MySpecialObject
the instance of JLP shouldn't be able to read public data and the BEV shouldn't be able to call member functions

This should be as type-safe as possible.

A solution I see involves wrappers :)

you write the code for MySpecialObject as you would normally, ignoring this extra requirement
you write a wrapper for each aspect of your class you wish to restrict. Say, MySpecialObject_Read and MySpecialObject_Execute.
these wrappers forward the requests (method calls, getter/setters) to an underlying shared_ptr of MySpecialObject
your MySpecialObject, MySpecialObject_Read and MySpecialObject_Execute classes each have (as needed) a "convert to ..." method. This method returns an appropriate wrapper over the underlying MySpecialObject

This solution provides a type-safe accessor with the desired limitations.

Each client class can choose what kind of access it needs. (And since you write these classes, you know what access you need.) If that is not acceptable, you could add factories that only create wrappers with certain limitations depending on a "token". That token might be the RTTI information of the calling class instance, although this could be abused.

Does this solve the original problem?

EDIT:

Remember that since you are the programmer you can instantiate an A whenever you want. By adding your class to a friend list or whatever...

What this solution provides is a clearer interface. Removing the explicit conversions probably makes it safer but less flexible. But, since they are explicit, you can easily look for them in the code and treat them as signs your architecture has a flaw somewhere.

Specifically, you can have code like this:

shared_ptr<A> * a = new A(parameters);

A_read aRead(a);
A_execute aExec(a);
A_write aWrite(aExec);

logger->Log(aRead);
view->SetUserData(aWrite);
controller->Execute(aExec);

Here there is an explicit conversion between an execute wrapper and a write wrapper, but you can decide on this based on your specific requirements.

But, with little effort (knowing which conversions are valid), just by looking at the call locations you can see that (with confidence!):

the logger will not change the state of your A
the view will not call methods on your A (other than setters)

This is true even if those particular method calls end up calling hundreds of other methods, more than you'd like to examine by hand.

At the cost of a few thin wrappers you gain the ability to see at a glance what a particular function call will do with the parameters you send. This may help a lot during debugging by helping you eliminate some branches from your investigation and, in general, would help people trying to understand the program.

I couldn't really find other reasons for using this ACL idea, at least ones where the costs don't outweigh the benefits. However, it does seem more intuitive than the visitor solution mentioned in the other answer.

Best Answer

Related Solutions

Should I strictly follow every single HTML and CSS Standard

Object-oriented – Filesystem like permissions for C++ type-members

Related Topic