C++/Qt Functions – Should All Arguments Be Checked for Null Values?

ccoding-stylenullparametersqt

Backstory

While developing with Qt Signal/Slots, I came across a few segmentation faults that had me puzzled as to what was causing it. Eventually I figured out that you could actually pass a slot function without providing it any of its arguments, which of course would cause a Segmentation Fault if the argument was used.

To avoid this pitfall, I am getting into the habit of checking all my arguments to make sure they are not NULL:

void MyClass::setMyString(QString input)
{
    if (input.isNull()) {
        qDebug() << "Error: setMyString() received NULL argument";
        return;
    }

    m_MyString = input;
    emit myStringChanged();
}

I'm thinking I ought to do this with all my functions from now on. Part of the reasoning is that I figure that it can not hurt to do this, and that if I or someone else ever needs to re-factor my code, it is less likely they will omit a necessary null value check.

Questions:

  1. Is this an acceptable style in all or most circumstances?
  2. Should I be introducing a runtime error, halting the program, instead of simply returning the function?
  3. Does this style of coding improve security?
  4. Does this style of coding improve stability?
  5. Are there any performance considerations, to the extent that checking for null values may take longer depending on the type?
  6. Beyond c++ and Qt, what other languages benefit from this style?
  7. Are there any significant drawbacks worth considering?

Best Answer

Kind of echoing Basile here but with a slightly more negative tone. I'd say "yes" too, but be careful with that if you are allowing silent violations of preconditions. As a personal example, I worked in a C codebase that did this kind of stuff through an API:

int f(struct Foo* foo)
{
    if (!foo)
        return error;
    ...
    return success;
}

Perhaps even with a debug log in the middle. Yet it was a massive codebase, tens of millions of lines of code, over 30 years of development, and with pretty shoddy standards. The warnings in places like logs were filled to the brim with developers having developed the habit of ignoring the output.

I found, most unfortunately, when trying to change such error checks into obnoxiously-loud assertions that there were thousands of places in the system that were violating these preconditions and simply ignoring the errors (if there were any reported, some functions just returned without doing anything in those cases). The developers just worked around it and kept trying stuff and adding more code until their code "worked", blissfully unaware of their original mistake that they were passing nulls to a function that did not allow it.

As a result, I strongly recommend doing like this:

void MyClass::setMyString(QString input)
{
    assert(!input.isNull() && "Error: setMyString() received NULL argument.");
    m_MyString = input;
    emit myStringChanged();
}

This will bring your application to a grinding halt if the assertion fails, showing you the precise source file and line number in which the assertion failed along with this message.

This is for cases where your function is not allowed to receive null (in your case, "null" here means "empty string"). For ones that are allowed, you might simply do something else, but it wouldn't be reporting a bug (for that, use the loud and obnoxious assert which cannot be ignored).

An example of where you might use if might be like this:

bool do_something(int* out_count = 0)
{
    ...
    if (out_count)
       *out_count = ...; // write the result back.
    return true;
}

For ones that are not allowed to receive a null, use references when possible. However, you might still encounter "nulls" if you accept smart pointers, e.g., or if you expand your definition of "null" to include empty strings as another example. In those cases, assert your preconditions liberally if you can. assert has the advantage of not only bringing your application to a grinding halt (good thing), but also pointing out where it was brought to a grinding halt even outside of a debugging session (but still with a debug build, e.g.), and it's also an effective means of documentation.

qDebug is a little too silent usually. A team with good habits that never allows those outputs to be ignored might get away with keeping those to zero. But an assert will protect you even against the sloppiest teams by bringing the entire process to a grinding halt, making these programmer errors impossible to ignore. Another minor benefit of assert is that it can't slow down your release (production) builds, while if (...) qDebug(...) will.

For errors caused by external sources outside of your control (ex: failing to connect to a server that was down, failing to allocate a huge block of memory, trying to read from a file the user tried to load that was corrupt, etc), throw exceptions, but not for programmer bugs like accessing an array out of bounds or receiving a null in a place that never should receive null (unless you're working in a very mission-critical software where the software attempts to gracefully recover from even programmer bugs as opposed to making them as easy to detect and reproduce as possible). For these cases, assert.

Related Topic