Despite being a programmer, I make software that goes in big things, which means that my workplace frequently involves the operation of power tools. My co-workers… discourage me from using the power tools. I'm not the handiest of people, and thus watching me work is awkward, uncomfortable, and creates a sense of danger. I'm allowed to use impact drivers and soldering irons, but when it comes time to use some of the more complex power saws, people gently nudge me aside.

There are tools that are valuable, but are just dangerous in the hands of the inexperienced. And even if they don't hurt themselves, you might end up boggling at the logic which made them use the tool that way. I'm talking, of course, about pre-compiler macros.

Lucas inherited some C++ code that depends on some macros, like this one:

#define CONDITION_CHECK(condition, FailureOp) if(!(condition)){FailureOp;}

This isn't, in-and-of-itself, terrible. It could definitely help with readability, especially with the right conditions. So what does it look like in use?

switch( transaction_type )
{
    case TYPE_ONE:
    case TYPE_TWO:
        return new CTransactionHandlerClass(/*stuff*/);
    default:
        break;
}
CONDITION_CHECK( false, return NULL );

Oh.

Even if we generously wanted to permit the use of literal true/false flags as some sort of debugging flag, this clearly isn't one of the situations where that makes sense. We always want to return NULL. There's never a time where we'd flip that flag to true to not return NULL.

CSpecificTransactionClass* pTrans = dynamic_cast< CAbstractTransactionClass* >( command );
if ( pTrans )
{
    CONDITION_CHECK( pTrans, return false);
    //stuff…
}

So, we do a dynamic cast, which if it fails is going to return a null value. So we have to check to see if it succeeded, which is what our if statement is doing. Once we know it succeeded, we immediately check to see if it failed.

In this case, that CONDITION_CHECK is just useless. But why be useless when you can also be too late?

CBaseCustomer *pCustomer;
CRetailCustomer *pRetail = new CRetailCustomer;
pCustomer = pRetail;
pCustomer->SetName( pName );
CONDITION_CHECK( pRetail, return false );

So, here, we have a safety check… after we interact with the possibly-not-intitialized object. Better late than never, I suppose.

Once, at work, someone handed me a bit of lumber and a hand saw, and told me to cut it. So I started, and after about a minute of watching me fail, they pointed out that the way I'd supported the lumber was causing it to bind the saw and pinch, because I had no clue what I was doing.

Which is to say: I think the developer writing and using this macro is much like me and the handsaw. It should be simple, it should be obvious, but when you have no clue what you're doing, you might not hurt yourself, but you'll make your co-workers laugh at you.

[Advertisement] Keep the plebs out of prod. Restrict NuGet feed privileges with ProGet. Learn more.