How do you validate an object's internal state?

Question

I'm interested in hearing what technique(s) you're using to validate the internal state of an object during an operation that, from it's own point of view, only can fail because of bad internal state or invariant breach.

My primary focus is on C++, since in C# the official and prevalent way is to throw an exception, and in C++ there's not just one single way to do this (ok, not really in C# either, I know that).

Note that I'm not talking about function parameter validation, but more like class invariant integrity checks.

For instance, let's say we want a Printer object to Queue a print job asynchronously. To the user of Printer, that operation can only succeed, because an asynchronous queue result with arrive at another time. So, there's no relevant error code to convey to the caller.

But to the Printer object, this operation can fail if the internal state is bad, i.e., the class invariant is broken, which basically means: a bug. This condition is not necessarily of any interest to the user of the Printer object.

Personally, I tend to mix three styles of internal state validation and I can't really decide which one's the best, if any, only which one is absolutely the worst. I'd like to hear your views on these and also that you share any of your own experiences and thoughts on this matter.

The first style I use - better fail in a controllable way than corrupt data:

void Printer::Queue(const PrintJob& job)
{
    // Validate the state in both release and debug builds.
    // Never proceed with the queuing in a bad state.
    if(!IsValidState())
    {
        throw InvalidOperationException();
    }

    // Continue with queuing, parameter checking, etc.
    // Internal state is guaranteed to be good.
}

The second style I use - better crash uncontrollable than corrupt data:

void Printer::Queue(const PrintJob& job)
{
    // Validate the state in debug builds only.
    // Break into the debugger in debug builds.
    // Always proceed with the queuing, also in a bad state.
    DebugAssert(IsValidState());

    // Continue with queuing, parameter checking, etc.
    // Generally, behavior is now undefined, because of bad internal state.
    // But, specifically, this often means an access violation when
    // a NULL pointer is dereferenced, or something similar, and that crash will
    // generate a dump file that can be used to find the error cause during
    // testing before shipping the product.
}

The third style I use - better silently and defensively bail out than corrupt data:

void Printer::Queue(const PrintJob& job)
{
    // Validate the state in both release and debug builds.
    // Break into the debugger in debug builds.
    // Never proceed with the queuing in a bad state.
    // This object will likely never again succeed in queuing anything.
    if(!IsValidState())
    {
        DebugBreak();
        return;
    }

    // Continue with defenestration.
    // Internal state is guaranteed to be good.
}

My comments to the styles:

1. I think I prefer the second style, where the failure isn't hidden, provided that an access violation actually causes a crash.
2. If it's not a NULL pointer involved in the invariant, then I tend to lean towards the first style.
3. I really dislike the third style, since it will hide lots of bugs, but I know people that prefers it in production code, because it creates the illusion of a robust software that doesn't crash (features will just stop to function, as in the queuing on the broken Printer object).

Do you prefer any of these or do you have other ways of achieving this?

Answer 1

You can use a technique called NVI (Non-Virtual-Interface) together with the template method pattern. This probably is how i would do it (of course, it's only my personal opinion, which is indeed debatable):

class Printer {
public:
    // checks invariant, and calls the actual queuing
    void Queue(const PrintJob&);
private:
    virtual void DoQueue(const PringJob&);
};


void Printer::Queue(const PrintJob& job) // not virtual
{
    // Validate the state in both release and debug builds.
    // Never proceed with the queuing in a bad state.
    if(!IsValidState()) {
        throw std::logic_error("Printer not ready");
    }

    // call virtual method DoQueue which does the job
    DoQueue(job);
}

void Printer::DoQueue(const PrintJob& job) // virtual
{
    // Do the actual Queuing. State is guaranteed to be valid.
}

Because Queue is non-virtual, the invariant is still checked if a derived class overrides DoQueue for special handling.

---

To your options: I think it depends on the condition you want to check.

If it is an internal invariant

If it is an invariant, it should not be possible for a user of your class to violate it. The class should care about its invariant itself. Therefor, i would assert(CheckInvariant()); in such a case.

It's merely a pre-condition of a method

If it's merely a pre-condition that the user of the class would have to guarantee (say, only printing after the printer is ready), i would throw std::logic_error as shown above.

I would really discourage from check a condition, but then doing nothing.

---

The user of the class could itself assert before calling a method that the pre-conditions of it are satisfied. So generally, if a class is responsible for some state, and it finds a state to be invalid, it should assert. If the class finds a condition to be violated that doesn't fall in its responsibility, it should throw.

Answer 2

The question is best considered in combination with how you test your software.

It's important that hitting a broken invariant during testing is filed as a high severity bug, just as a crash would be. Builds for testing during development can be made to stop dead and output diagnostics.

It can be appropriate to add defensive code, rather like your style 3: your DebugBreak would dump diagnostics in test builds, but just be a break point for developers. This makes less likely the situation where a developer is prevented from working by a bug in unrelated code.

Sadly, I've often seen it done the other way round, where developers get all the inconvenience, but test builds sail through broken invariants. Lots of strange behaviour bugs get filed, where in fact a single bug is the cause.

Answer 3

It's a fine and very relevant question. IMHO, any application architecture should provide a strategy to report broken invariants. One can decide to use exceptions, to use an 'error registry' object, or to explicitly check the result of any action. Maybe there are even other strategies - that's not the point.

Depending on a possibly loud crash is a bad idea: you cannot guarantee the application is going to crash if you don't know the cause of the invariant breach. In case it doesn't, you still have corrupt data.

The NonVirtual Interface solution from litb is a neat way to check invariants.

Answer 4

Tough question this one :)

Personally, I tend to just throw an exception since I'm ussually too much into what Im doing when implementing stuff to take care of what should be taken care of by your design. Ussually this comes backs and bites me later on...

My personal experience with the "Do-some-logging-and-then-dont-do-anything-more"-strategy is that it too comes back to bite you - especially if it's implemented like in your case (no global strategy, every class could potentially do it different ways).

What I would do, as soon as I discovered a problem like this, would be to speak to the rest of my team and tell them that we need some kind of global error-handling. What the handling will do depends on your product (you don't want to just do nothing and log something in a subtle developer-minded file in an Air Traffic Controller-system, but it would work fine if you were making a driver for, say, a printer :) ).

I guess what Im saying is, that imho, this question is something you should resolve on a design-level of your application rather than at implementation level. - And sadly there's no magic solutions :(