Double Negatives

This beauty was submitted by the amazing and fast rEx.

The culprit was none other than fellow CSOD hunter-gatherer* J-Ra.

if ( IsBadSSN() == false )
{
    Result = stBadPersonSSN;
}

Without knowing what IsBadSSN does, one would assume (based on what it sounds like it would do) that this is an error any person with a basic understanding of the English language should be able to catch.

Moral of the story?

Double Negatives in if statements are hard to read.

This would have been better written

if ( ValidSSN() == false )
{
    Result = stBadPersonSSN;
}

* hunting and gathering CSOD's for your enjoyment.

And I, I Took the Return Path Less Traveled By

And that has made all the difference.

When you get into the mind-set that multiple return-paths are ok you run risks.

Pre-Conditions are usually ok, and not a horrible idea, unless implemented part-way through the function.

When you get into the mind-set that multiple return-paths are ok, you run the risk of writing (and possibly maintaining) code that does the following. And if you're especially unlucky, (like the original programmer) your code is being used in a Windows service where people expect things to run for months between restarts.

bool TSomeClass::SomeFunction()
{
    long RECORD_ID = GetRecordID();

    if( RECORD_ID )
    {
        THandlerClass *Handler = new THandlerClass();

        Handler->LoadRecord( RECORD_ID );

        if( Handler->SomeBoolFlag == false  )
        {
            ReportError( "Some Bool Flag is false - can't continue" );
            return false;
        }

        Handler->Process();

        delete Handler;

        return true;
    }
    else
    {
        ReportError( "Record ID is not set - unable to process." );
    }

    return false;
}

Moral of the story?

Pre-Conditions should be before any memory allocation.

Also, if possible, create the memory in the constructor of a class and destroy it in the destructor - this way you don't have to worry about leaking memory inside functions, and unless you have a situation I ran into the other day where the destructor wasn't being called (the subject of a future posting) the memory will be released when the host object is destroyed.

Comments are cool, Comments are fun...

Comments make their food from the rays of the sun.

Coding is more fun with people who are willing to leave comments in the code like this one:

// to all evildoers, this SQL statement
// must accurately reflect whether or
// not there is a DB connection.
// if it is ever trying to access a table
// that doesn't exist, it will always
// return false. do not change unless
// you know what you are doing.

This was in a function where I was getting an "access in invalid memory" error from Borland's CodeGuard.

This was due to the fact that this base dll is used in applications that don't create the base database object because they don't operate on the database at all.

A simple

if ( bool( Object ) )
{
    // logic here to determine if the database is connected
}
else
{
    // logic here to handle the lack of database functionality
}

would have helped.

Moral of the story?
Never assume you have a valid instance of a global variable... especially in a statically linked dll that relies on the global variable being created in the main application.

I love virtual functions

Pure Virtual functions are even better - allowing you to force a child class to implement a particular function.

Imagine my surprise when I found this brand new type of pure virtual function:

long TSomeClass::SomeFunction()
{
    // This is pure virtual
    return 0;
}

The header of this class was even correct.

    virtual long SomeFunction() = 0;

Now, you may laugh at me - at my nerdish geekiness, but when I saw this, I started laughing out loud and had to print it off and show the other programmers in the office.

In fact, it was this snippet that caused me to start this blog.

So, there you go... let's laugh at this code, but most of all, let's laugh at the original programmer... you know who you are.