68

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:

  1. Mandating NULL guards for every single dereferenced pointer is overkill, but
  2. You can side-step the whole debate by using a reference instead (if possible) or a const pointer, and
  3. assert statements are a more enlightened alternative to NULL guards for verifying that a function's preconditions are met.
evadeflow
  • 1,222
  • 16
    Don't think for a minute that mistakes are the domain of junior programmers. 95% of developers of all levels of experience goof something up once in awhile. The remaining 5% are lying through their teeth. – Blrfl Nov 02 '13 at 17:09
  • 58
    If I saw someone write code that checks for a null and silently fails, I'd want to fire them on the spot. – Winston Ewert Nov 02 '13 at 17:10
  • Can you use smart pointers std::unique_ptr and const-qualify them to prevent this type of errors? That is, require the member reference (pointer) to be valid at all times, except in the constructor or destructor. – rwong Nov 02 '13 at 23:36
  • 18
    Stuff failing silently is pretty much the worst. At least when it blows up you know where the explosion occurs.

    Checking for null and doing nothing is just a way to move the bug on down the execution, making it far more difficult to track back to the source.

     – MirroredFate Nov 03 '13 at 00:28
  • @MirroredFate: there are multiple approaches which all lead to "failing silently / unpredictably". Calling a virtual method on a null reference can also make it difficult to track back to the source. (In fact, nothing short of a logging and error-handling system would prevent it. OP already indicates this is not an option because OP makes non-user-serviceable consumer electronics.) – rwong Nov 03 '13 at 01:16
  • 1
    +1, very interesting question. In addition to my answer, I'd also point out that when you have to write code who's purpose is to accommodate a unit test, what you're really doing is trading increased risk for a false sense of security (more lines of code = more possibilities for bugs). Redesigning to use references not only improves the readability of the code, but also reduces the amount of code (and tests) that you have to write in order to prove that it works. – Seth Nov 03 '13 at 01:54
  • If you're suggesting that testability concerns shouldn't be a huge driver in decisions about code structure, I can't say I agree with you. The IMsgSender class in the example above is a structural concession made solely for testing. Only two concrete subclasses exist: the 'real' one that talks to the network, and a fake for testing. Setting components up so that 'awkward collaborators' can be mocked out during testing is just good practice, IMHO. I inject dependencies by reference whenever I can, but (as noted in my comment to your answer) this isn't always possible. – evadeflow Nov 03 '13 at 16:14
  • 1
    @WinstonEwert There are cases where you want a silent fail. My current tech stack has several. Immediate fire is a bit harsh but in general programming you don't want a silent fail. Some models want it though. – Rig Nov 03 '13 at 22:40
  • 6
    @Rig, I'm sure that there are appropriate cases for a silent fail. But if someone puts silent fails in as general practice (unless working in a realm where it make sense), I really don't want to be working on a project they are putting code in. – Winston Ewert Nov 03 '13 at 22:59
  • 1
    This refers to Java, but is still highly relevant: http://thecodelesscode.com/case/115 – Parker Coates Apr 09 '15 at 16:55
  • If you check for null this way, then what do you put in the "else"? – RemcoGerlich Jan 07 '16 at 09:05
  • If msgSender_ is not null always so refactor it to reference not pointer – fnc12 Jan 15 '19 at 10:03

13 Answers13

78

I feel the code should read:

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

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

This is actually better than guarding the NULL, because it makes it very clear that the function should never be called if msgSender_ is NULL. It also makes sure that you'll notice if this happens.

The shared "wisdom" of your colleagues will silently ignore this error, with unpredictable results.

In general bugs are easier to fix if they're detected closer to their cause. In this example the proposed NULL guard would lead to a shutdown message not getting set, which may or may not result in a noticeable bug. You'd have a harder time working backwards to the SignalShutdown function than if the entire application just died, producing a handy-dandy backtrace or core dump pointing directly to SignalShutdown().

It's a little counterintuitive, but crashing as soon as anything is wrong tends to make your code more robust. That's because you actually find the problems, and tends to have very obvious causes as well.

gnat
  • 21,213
  • 29
  • 113
  • 291
Kristof Provost
  • 897
  • 10
    The big difference between calling assert and the OP's code example is that assert is only enabled in debug builds (#define NDEBUG) When the product gets into customer hands, and debug is disabled, and a never-yested combination of code paths executes to leave the pointer null even though the function in question is executed, the assert provides you no protection. – atk Nov 02 '13 at 12:09
  • 17
    Presumably you'd have tested the build before actually sending it to the customer, but yes, this is a possible risk. Solutions are easy though: either just ship with asserts enabled (that's the version you tested anyway) or replace it with your own macro which asserts in debug mode and just logs, logs+backtraces, exits, ... in release mode. – Kristof Provost Nov 02 '13 at 13:01
  • @atk That's why I'd put asserts, exceptions, and nullptr checks (with else statements that fail loudly). If there's a problem SOMEONE will find it. Even if the last person to do so is the customer. – Casey Nov 02 '13 at 18:26
  • Asserts are useless when the code is not running on the same machine as your debugger unless you have expensive hardware. You cannot recover from failed asserts and so the user will probably have to reboot their device, you can however handle exceptions. – James Nov 02 '13 at 19:10
  • @KristofProvost: It is impossible to fully test an application of any real complexity with every possible code path using every possible input on every possible platform with every possible operating system with every possible hardware pligin. Even when just limiting yourself yourself to supported platforms and hardware. Customers always find new things to do with your product that testing simply cannot cover. – atk Nov 02 '13 at 19:44
  • 7
    @James - in 90% of case you won't recover from the failed NULL check as well. But in such case the code will silently fail and error may appear much later on - for example you thought that you saved to file but actually the null check sometimes fails leaving you non the wiser. You cannot recover from all situation that must not happen. You can verify that they won't happen, but I don't think you have a budget unless you write code for NASA satellite or nuclear power plant. You do need to have a way of saying "I have no idea what's going on - the program is not suppose to be in this state". – Maja Piechotka Nov 02 '13 at 20:14
  • Also in most cases while the program cannot recover from failed assert the OS can recover from failed assert in application. So for most cases users won't need to reboot the device - just start the application once again (and presumably it is very rare as your testing catched most of the errors). – Maja Piechotka Nov 02 '13 at 20:16
  • 1
    @MaciejPiechotka The OP said the software was for an embedded system. Restarting the application may require removing a bettery or unplugging it. Also, nowhere did I say to silently fail. Throwing is far better than asserting. And if you do not catch the exception you essentially get an assert anyway. – James Nov 03 '13 at 00:34
  • 6
    @James I disagree with throwing. That makes it look like it's something that can actually happen. Asserts are for things which are supposed to be impossible. For real bugs in the code in other words. Exceptions are for when unexpected, but possible, things happen. Exceptions are for things you could conceivable handle and recover from. Logic errors in the code you can't recover from, and you shouldn't try either. – Kristof Provost Nov 03 '13 at 09:22
  • @atk: if they're willing to manually null-check before every dereference, they're already paying the overhead and should be willing to leave asserts enabled in release builds. Specifically for null checks, there is little point asserting — just dereference it and it will crash if NULL (with zero overhead). – Beni Cherniavsky-Paskin Nov 03 '13 at 10:19
  • @BeniCherniavsky-Paskin Well, an assert will typically generate output (file/line number) before it aborts, so that's an advantage. Also, if you're dealing with very large structures or tables there's a chance you're not going to crash instantly. Mostly the overhead of asserts is not going to matter. – Kristof Provost Nov 03 '13 at 10:25
  • @KristofProvost Good point, I forgot that ptr->foo where ptr == null doesn't necessarily access address (or page) 0. – Beni Cherniavsky-Paskin Nov 03 '13 at 10:32
  • 2
    @James: In my experience with embedded systems, the software and hardware are invariably designed such that is is extremely hard to render a device completely unusable. In the large majority of cases, there is a hardware watchdog that forces a complete reset of the device if the software stops running. – Bart van Ingen Schenau Nov 03 '13 at 11:31
  • @BeniCherniavsky-Paskin: Agreed - if they want it to crash, rather than gracefully handle the error. My original comment on this answer was because most people disable debug in release builds without realizing that asserts are also disabled. – atk Nov 03 '13 at 12:18
  • @James - I can't see any reference to embedded system but as Bart van Ingen Schenau said - the watchdog should take care about it - and clean restart is probably preferable to having increasingly inconsistent state in long run. – Maja Piechotka Nov 03 '13 at 16:00
  • 2
    I'd throw at points where invalid input can be passed in through a public API (in this case in the constructor) and assert for internal consistency checks (I know it'll never be null because I already checked in the constructor). – CodesInChaos Nov 04 '13 at 09:22
  • @CodesInChaos: Sure, do throw if that's part of the contract. But please don't do that if it isn't. – Deduplicator Jan 06 '16 at 23:37
69

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.

aaronps
  • 810
  • 4
    +1 for a very succinct answer that pretty much sums up the way I feel: "it depends..." You also had the guts to say "never check for null" (if null is invalid). Placing an assert() in every member function that dereferences a pointer is a compromise that will likely placate a lot of people, but even that feels like 'speculative paranoia' to me. The list of things beyond my ability to control is infinite, and once I allow myself to start worrying about them, it becomes a really slippery slope. Learning to trust my tests, my colleagues—and my debugger—has helped me sleep better at night. – evadeflow Nov 02 '13 at 15:29
  • 2
    When you're reading code those asserts can be very helpful in understanding how the code is supposed to work. I've never encoutered a codebase that made me go 'I wish it didn't have all these asserts all over the place.', but I've seen plenty where I said the opposite. – Kristof Provost Nov 02 '13 at 17:45
  • 1
    Plain and simple, this is the correct answer to the question. – Matthew Azkimov Nov 02 '13 at 18:55
  • 2
    This is the closest correct answer but i can't agree with 'never check for null', always check for invalid parameters at the point they'll be assigned to a member variable. – James Nov 02 '13 at 19:08
  • Thanks for the comments. It is better to let it crash sooner if someone doesn't read the specification and initializes it with null value when it must have something valid there. – aaronps Nov 03 '13 at 02:31
  • @James: And that implicitly means that you must have defined which possible values are invalid. E.g. for double you may want to check that the number is finite. – MSalters Nov 04 '13 at 08:47
32

If msgSender must never be null, you should put the null check only in the constructor. This is also true for any other inputs into the class - put the integrity check at the point of entry into the 'module' - class, function etc.

My rule of thumb is to perform integrity checks between module boundaries - the class in this case. In addition, a class should be small enough that it is possible to quickly mentally verify the integrity of the lifetimes of the class members - ensuring that such mistakes as improper deletes / null assignments are prevented. The null check that is performed in the code in your post assumes that any invalid usage actually assigns null to the pointer - which is not always the case. Since 'invalid usage' inherently implies that any assumptions about regular code does not apply, we cannot be sure to catch all types of pointer errors - for example an invalid delete, increment etc.

In addition - if you are certain the argument can never be null, consider using references, depending on your usage of the class. Otherwise, consider using std::unique_ptr or std::shared_ptr in lieu of a raw pointer.

Max
  • 2,039
11

No, it is not reasonable to check each and every pointer dereference for the pointer being NULL.

Null-pointer checks are valuable on function arguments (including constructor arguments) to ensure preconditions are met or to take appropriate action if an optional parameter is not provided, and they are valuable to check a class's invariant after you have exposed the class's internals. But if the only reason for a pointer to have become NULL is the existence of a bug, then there is no point in checking. That bug could just as easily have set the pointer to another invalid value.

If I were faced with a situation like yours, I would ask two questions:

  • Why was the mistake from that junior programmer not caught before the release? For example in a code review or in the testing phase? Bringing a faulty consumer electronics device to the market can be just as costly (if you take market share and goodwill into account) as releasing a faulty device used in a safety-critical application, so I would expect the company to be serious about testing and other QA activities.
  • If the null-check fails, what error handling do you expect me to put in place, or could I just write assert(msgSender_) instead of the null check? If you just put the null check in, you might have prevented a crash, but you might have created a worse situation because the software continues on the premise that an operation has taken place while in reality that operation was skipped. This might lead to other parts of the software becoming unstable.
Bart van Ingen Schenau
  • 74,630
10

This example seems to be more about object lifetime than whether or not an input parameter is null†. Since you mention that the PowerManager must always have a valid IMsgSender, passing the argument by pointer (thereby allowing the possibility of a null pointer) strikes me as a design flaw††.

In situations like this, I would prefer to change the interface so the caller's requirements are enforced by the language: 

PowerManager::PowerManager(const IMsgSender& msgSender)
  : msgSender_(msgSender) {}

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

Rewriting it this way says that PowerManager needs to hold a reference to IMsgSender for it's entire lifetime. This, in turn, also establishes an implied requirement that IMsgSender must live longer than PowerManager, and negates the need for any null pointer checking or assertions inside PowerManager.

You can also write the same thing using a smart pointer (via boost or c++11), to explicitly force IMsgSender to live longer than PowerManager: 

PowerManager::PowerManager(std::shared_ptr<IMsgSender> msgSender) 
  : msgSender_(msgSender) {}

void PowerManager::SignalShutdown() {
    // Here, we own a smart pointer to IMsgSender, so even if the caller
    // destroys the original pointer, we still have a valid copy
    msgSender_->sendMsg("shutdown()");
}

This method is prefered if it is possible that IMsgSender's lifetime can not be guaranteed to be longer than PowerManager's (i.e., x = new IMsgSender(); p = new PowerManager(*x);).

† With regard to pointers: rampant null checking makes code harder to read, and does not improve stability (it improves the appearance of stability, which is much worse).

Somewhere, someone got an address for memory to hold the IMsgSender. It is that function's responsibility to make sure that the allocation succeeded (checking library return values or properly handling std::bad_alloc exceptions), so as to not pass around invalid pointers.

Since the PowerManager does not own the IMsgSender (it's just borrowing it for a while), it is not responsible for allocating or destroying that memory. This is another reason why I prefer the reference.

†† Since you're new at this job, I expect that you're hacking on existing code. So by design flaw, I mean that the flaw is in the code that you're working with. Thus, the people who are flagging your code because it's not checking for null pointers are really flagging themselves for writing code that requires pointers :)

Seth
  • 209
  • 1
    There's actually nothing wrong with using raw pointers sometimes. Std::shared_ptr is not a silver bullet and using it in an argument list is a cure for sloppy engineering although I realise it's considered 'leet' to use it whenever possible. Use java if you feel like that. – James Nov 03 '13 at 03:22
  • 2
    I didn't say raw pointers are bad! They definitely have their place. However, this is C++, references (and shared pointers, now) are part of the language for a reason. Use them, they will cause you to succeed. – Seth Nov 03 '13 at 03:54
  • I like the smart pointer idea, but it's not an option in this particular gig. +1 for recommending the use of references (as @Max and a few others have). Sometimes it's not possible to inject a reference, though, due to chicken-and-egg dependency issues, i.e., if an object that would otherwise be a candidate for injection needs to have a pointer (or reference) to its holder injected into it. This can sometimes indicate a tightly coupled design; but it's often acceptable, as in the relationship between an object and its iterator, where it never makes sense to use the latter without the former. – evadeflow Nov 03 '13 at 14:35
8

Like exceptions, guard conditions are only useful if you know what to do to recover from the error or if you want to give a more meaningful exception message.

Swallowing an error (whether caught as an exception or a guard-check), is only the right thing to do when the error doesn't matter. The most common place for me to see errors being swallowed is in error logging code -- you don't want to crash an app because you were unable to log a status message.

Anytime a function is called, and it's not optional behavior, it should fail loudly, not silently.

Edit: on thinking about your junior programmer story it sounds like what happened was that a private member was set to null when that was never supposed to be allowed to happen. They had a problem with inappropriate writing and are attempting to fix it by validating upon reading. This is backwards. By the time you identify it, the error has already happened. The code-review/compiler enforceable solution for that isn't guard conditions, but instead getters and setters or const members.

jmoreno
  • 10,853
  • 1
  • 31
  • 48
7

As other have noted, this depends on whether or not msgSender can be legitimately NULL. The following assumes that it should never be NULL.

void PowerManager::SignalShutdown()
{
    if (!msgSender_)
    {
       throw SignalException("Shut down failed because message sender is not set.");
    }

    msgSender_->sendMsg("shutdown()");
}

The proposed "fix" by the others on your team violates the Dead Programs Tell No Lies principle. Bugs are really hard to find as it is. A method that silently changes its behavior based on an earlier problem, not only makes it hard to find the first bug but also adds a 2nd bug of its own.

The junior wreaked havoc by not checking for a null. What if this piece of code wreaks havoc by continuing to run in an undefined state (device is on but the program "thinks" it's off)? Perhaps another part of the program will do something that is only safe when the device is off.

Either of these approaches will avoid silent failures:

  1. Use asserts as suggested by this answer, but make sure they are turned on in production code. This, of course, could cause problems if other asserts were written with the assumption that they would be off in production.

  2. Throw an exception if it is null.

Community
  • 1
poke
  • 560
5

I agree with trapping the null in the constructor. Further, if the member is declared in the header as:

IMsgSender* const msgSender_;

Then the pointer cannot be changed after initialisation, so if it was fine at construction it will be fine for the lifetime of the object containing it. (The object pointed to will not be const.)

Grimm The Opiner
  • 284
  • That is great advice, thank you! So good, in fact, that I'm sorry I can't vote it up any higher. It's not a direct answer to the stated question, but it is a great way to eliminate any possibility of the pointer 'accidentally' becoming NULL. – evadeflow Nov 05 '13 at 15:17
  • @evadeflow I thought "I agree with everyone else" answered the main thrust of the question. Cheers though! ;-) – Grimm The Opiner Nov 05 '13 at 15:34
  • It would be a little impolite of me to change the accepted answer at this point, given the way the question was phrased. But I really think this bit of advice is outstanding, and I'm sorry I didn't think of it myself. With a const pointer, there's no need to assert() outside the constructor, so this answer seems even a bit better than @Kristof Provost's (which was also outstanding, but addressed the question somewhat indirectly as well.) I hope others will vote this up, since it really doesn't make sense to assert in every method when you can just make the pointer const. – evadeflow Nov 05 '13 at 23:05
  • @evadeflow People only visit Questins for the rep, now it's been answered no-one'll look at it again. ;-) I only popped in because it was a question about pointers and I'm keeping an eye out for the bloody "Use smart pointers for everything always" brigade. – Grimm The Opiner Nov 06 '13 at 08:42
3

This is Outright Dangerous!

I worked under a senior developer in a C codebase with the shoddiest "standards" who pushed for the same thing, to blindly check all pointers for null. The developer would end up doing things like this:

// Pre: vertex should never be null.
void transform_vertex(Vertex* vertex, ...)
{
    // Inserted by my "wise" co-worker.
    if (!vertex)
        return;
    ...
}

I once tried removing such a check of the precondition one time in such a function and replacing it with an assert to see what would happen.

To my horror, I found thousands of lines of code in the codebase which were passing nulls to this function, but where the developers, likely confused, worked around and just added more code until things worked.

To my further horror, I found this issue was prevalent in all sorts of places in the codebase checking for nulls. The codebase had grown over decades to come to rely on these checks in order to be able to silently violate even the most explicitly-documented preconditions. By removing these deadly checks in favor of asserts, all the logical human errors over decades in the codebase would be revealed, and we would drown in them.

It only took two seemingly-innocent lines of code like this + time and a team to end up masking a thousand accumulated bugs.

These are the kinds of practices that make bugs depend on other bugs to exist in order for the software to work. It's a nightmare scenario. It also makes every logical error related to violating such preconditions show up mysteriously a million lines of code away from the actual site in which the mistake occurred, since all these null checks just hide the bug and hide the bug until we reach a place that forgot to hide the bug.

To simply check for nulls blindly in every place where a null pointer violates a precondition is, to me, utter insanity, unless your software is so mission-critical against assertion failures and production crashes that the potential of this scenario is preferable.

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?

So I'd say, absolutely not. It's not even "safe". It may very well be the opposite and mask all kinds of bugs throughout your codebase which, over the years, can lead to the most horrific scenarios.

assert is the way to go here. Violations of preconditions should not be allowed to go unnoticed, or else Murphy's law can easily kick in.

  • 1
    "assert" is the way to go - but remember that on any modern system, each memory access through a pointer has a null-pointer assert built into the hardware. So in "assert (p != NULL); return *p; " even the assert is mostly pointless. – gnasher729 Jan 07 '16 at 00:55
  • @gnasher729 Ah, that's a very good point, but I tend to see assert as both a documentation mechanism to establish what the preconditions are and not merely a way to force an abort. But I also kind of like the nature of the assertion failure which shows you exactly which line of code caused a failure and the assert condition failed ("documenting the crash"). Can come in handy if we end up using a debug build outside of a debugger and get caught off-guard. –  Jan 07 '16 at 01:04
  • @gnasher729 Or I may have misunderstood a part of it. Do these modern systems show which line of source code in which the assertion failed? I'd generally imagine they lost the info at that point and might merely show segfault/access violation, but I'm kinda far from "modern" -- still stubbornly on Windows 7 for most of my development. –  Jan 07 '16 at 01:08
  • For example, on MacOS X and iOS, if your app crashes due to a null pointer access during development, the debugger will tell you the exact line of code where it happens. There is another strange aspect: The compiler will try to give you warnings if you do something suspicious. If you pass a pointer to a function and access it, the compiler won't give you a warning that the pointer might be NULL, because it happens so often, you would be drowned in warnings. However, if you do a comparison if (p == NULL) ... then the compiler assumes that there is a good chance that p is NULL, – gnasher729 Jan 07 '16 at 09:06
  • because why would you have tested it otherwise, therefore it will give a warning from then on if you use it without testing. If you use your own assert-like macro, you have to be a bit clever to write it in a way so that "my_assert (p != NULL, "It's a null pointer, stupid!"); *p = 1; " doesn't give you a warning. – gnasher729 Jan 07 '16 at 09:09
  • @gnasher729 Ah I see -- in the environments I worked in, we did some weird things -- like sometimes the QA and internal testing would run debug builds (but not through a debugger). It was handy to always get a source file line number in those cases, and possible attached message. –  Jan 07 '16 at 16:42
2

Objective-C, for example, treats every method call on a nil object as a no-op that evaluates to a zero-ish value. There are some advantages to that design decision in Objective-C, for the reasons suggested in your question. The theoretical concept of null-guarding every method call has some merit if it's well publicized and consistently applied.

That said, code lives in an ecosystem, not a vacuum. The null-guarded behaviour would be non-idiomatic and surprising in C++, and therefore should be considered harmful. In summary, nobody writes C++ code that way, so don't do it! As a counter-example, though, note that calling free() or delete on a NULL in C and C++ is guaranteed to be a no-op.

In your example, it would probably be worthwhile to place an assertion in the constructor that msgSender is non-null. If the constructor called a method on msgSender right away, then no such assertion would be necessary, since it would crash right there anyway. However, since it is merely storing msgSender for future use, it would not be obvious from looking at a stack trace of SignalShutdown() how the value came to be NULL, so an assertion in the constructor would make debugging significantly easier.

Even better, the constructor should accept a const IMsgSender& reference, which cannot possibly be NULL.

Community
  • 1
200_success
  • 1,578
1

The reason that you're asked to avoid null dereferences is to ensure that your code is robust. Examples of junior programmers long long ago are just examples. Anyone can break the code by accident and cause a null dereference - especially for globals and class globals. In C and C++ it's even more possible accidentally, with the direct memory management capability. You might be surprised, but this kind of thing happens very often. Even by very knowledgeable, very experienced, and very senior developers.

You don't need to null check everything, but you do need to protect against dereferences that have decent likelihood of being null. This is commonly when they are allocated, used and dereferenced in different functions. It is possible that one of the other functions will be modified and break your function. It is also possible that one of the other functions may be called out of order (like if you have a deallocator that can be called separately from the destructor).

I prefer the approach your coworkers are telling you in combination with using assert. Crash in the test environment so it's more obvious that there's a problem to fix and fail gracefully in production.

You should also be using a robust code correctness tool like coverity or fortify. And you should be addressing all compiler warnings.

Edit: as others have mentioned, silently failing, as in several of the code examples, is generally the wrong thing, as well. If your function cannot recover from the value being null, it should return an error (or throw an exception) to its caller. The caller is responsible for then either fixing its call order, recovering, or returning an error (or throwing an exception) to its caller, and so on. Eventually, either a function is able to gracefully recover and move on, gracefully recover and fail (such as a database failing a transaction because of an internal error for one user but not acutally exiting), or the function determines that the application state is corrupt and unrecoverable and the application exits.

atk
  • 115
  • +1 for having the courage to support a (seemingly) unpopular position. I would've been disappointed if nobody defended my colleagues on this (they're very smart people who have been putting out a quality product for many years). I also like the idea that apps should "crash in the test environment... and fail gracefully in production." I'm not convinced that mandating 'rote' NULL checks are the way to do that, but I appreciate hearing an opinion from someone outside my workplace who's basically saying: "Aigh. Doesn't seem too unreasonable." – evadeflow Nov 02 '13 at 14:57
  • I get annoyed when I see people testing for NULL after malloc(). It tells me they don't understand how modern OSs manage memory. – Kristof Provost Nov 02 '13 at 18:52
  • @KristofProvost care to expand on why checking if memory allocation succeeds annoys you? – James Nov 02 '13 at 19:03
  • @KristofProvost why would that annoy you? Wouldn't the user be even more annoyed if your program segfaulted because the system didn't have enough ram? – scott_fakename Nov 02 '13 at 19:07
  • @KristofProvost: why? I assume you are aware that malloc can, and on some occasions does return null. If you don't check, you'll never know if the malloc succeeded. Similarly, it can return null but actually allocate a very small amount of memory on failure on some systems. Again, if. You don't check, you might just have a buffer overrun and be corrupting memory. – atk Nov 02 '13 at 19:19
  • @gnat: thank you for the edit! I have a lot of trouble typing on my tablet (which has become my promary home device). Virtual keyboards seem to enable lots of errors... – atk Nov 02 '13 at 19:22
  • 2
    My guess is that @KristofProvost is talking about OSes that use overcommit, for which malloc always succeeds (but the OS may later kill your process if there isn't actually enough memory available). However, this is not a good reason to skip null checks on malloc: overcommit is not universal across platforms and even if it's enabled, there may be other reasons for malloc to fail (e.g., on Linux, a process address space limit set with ulimit). – John Bartholomew Nov 02 '13 at 20:31
  • 1
    What John said. I was indeed talking about overcommit. Overcommit is enabled by default on Linux (which is what pays my bills). I don't know, but suspect so, if it's the same on Windows. In most cased you're going to end up crashing anyway, unless you're prepared to write a lot of code that's never, ever going to be tested to handle errors that are almost certainly never going to happen... – Kristof Provost Nov 02 '13 at 21:02
  • 2
    Let me add that I've also never seen code (outside of the linux kernel, where out of memory is actuall realisticly possible) that will actually correctly handle an out-of-memory condition. All of the code that I've seen try would just crash later anyway. All that accomplished was to waste time, make the code harder to understand and hide the real problem. Null dereferences are easy to debug (if you have a core file). – Kristof Provost Nov 02 '13 at 21:24
  • And because you don't see people doing it right you think it'll make things better if we all give up? – atk Nov 02 '13 at 23:15
  • @KristofProvost complete nonsense. Also, the OP's code runs on an embedded device, debugging can be a lot harder and expensive. – James Nov 03 '13 at 03:16
  • @atk Because it doesn't work, it makes the rest of the code harder to understand, and usually out-of-memory doesn't happen they way you think it does. – Kristof Provost Nov 03 '13 at 09:19
  • @James Which bit? Memory overcommit is a real thing. Unless you deliberately turn it off, it is going to happen. And if you know enough to turn it off you know enough to ignore what I said too. – Kristof Provost Nov 03 '13 at 09:20
  • "should return an error to its caller": error codes are easy to ignore, leading again to silently failing. Throwing an exception is better. – Beni Cherniavsky-Paskin Nov 03 '13 at 10:27
  • @BeniCherniavsky-Paskin: Good point. i lump exceptions in with errors, but I edited to be more clear. – atk Nov 03 '13 at 12:14
  • @KristofProvost: I guess you and I will have to agree to disagree. I will continue to recommend safe programming and graceful handling of errors and you can continue recommending aticking your head in the sand and hoping errors don't happen. – atk Nov 03 '13 at 12:15
  • @KristofProvost: Most of the time I'm willing to live with a program that dies on out of memory, and for that not checking malloc() is acceptable. Sometimes I'm working on a program that must not die, and it needs to handle OOM conditions. You know you can exclude programs from OOM right? – Joshua Jan 07 '16 at 21:33
  • @Joshua Yes, that's possible. Of course, you'd have to make sure that all of the libraries you rely on (and all of the ones they rely on in turn) also handle out-of-memory correctly. It's possible, but I've never seen it done for non-trivial applications. – Kristof Provost Jan 08 '16 at 07:43
1

The use of a pointer instead of a reference would tell me that msgSender is only an option and here the null check would be right. The code snippet is too slow as to decide that. Maybe there are other elements in PowerManager that are valuable (or testable)...

When choosing between pointer and reference, I thoroughly weigh both options. If I have to use a pointer for a member (even for private members), I have to accept the if (x) procedure each time I dereferencing it.

Wolf
  • 640
  • As I mentioned in another comment somewhere, there are times when injecting a reference isn't possible. An example I run into a lot is where the held object itself needs a reference to the holder: – evadeflow Nov 05 '13 at 17:33
  • @evadeflow: OK, I confess, it depends on the class size and the visibility of pointer members (that cannot be made references), whether I check before dereferencing. In cases where the class is small and plain and the pointer(s) are private, I don't... – Wolf Nov 05 '13 at 18:50
0

It depends on what you want to happen.

You wrote that you are working on "a consumer electronics device". If, somehow, a bug is introduced by setting msgSender_ to NULL, do you want

  • the device to carry on, skipping SignalShutdown but continuing the rest of its operation, or
  • the device to crash, forcing the user to restart it?

Depending on the impact of the shutdown signal not being sent, option 1 might be a viable choice. If the user can continue to listen to his music, but the display still shows the title of the previous track, that might be preferable to a complete crash of the device.

Of course, if you choose option 1, an assert (as recommended by others) is vital to reduce the likelihood of such a bug creeping in unnoticed during development. The if null guard is just there for failure mitigation in production use.

Personally, I also prefer the "crash early" approach for production builds, but I am developing business software that can be fixed and updated easily in case of a bug. For consumer electronics devices, this might not be so easy.

Heinzi
  • 9,748