Code doesn’t have to be confusing …

Just a quick post today on some code I found that was a little confusing and a little annoying, here is the function.

//----------------------------------------------------------------------------
// SetFontName() - 
// Desc: Changes the font face used for printing
// params: The new face name IE Courier 
// Returns: he old font face
//----------------------------------------------------------------------------
LPCSTR CPrintClass::SetFontName(LPCSTR TheFontName)
{
  static char buffer[40];
  if (TheFontName != NULL) {
    m_PrintDescripter.FontName = TheFontName;
  }
  return buffer;
}

Looks legit, right, well let’s delve into it.  The name of the function indicates that it will set the font name of something, that seems fine, it takes a pointer to a string (char’s) as the font name, if this is not NULL the name is set, so far so good.

The if statement could have just been written as if (TheFontName), the != NULL is superfluous, not a biggie.  But what happens if the passed in name is NULL, well nothing happens and the caller knows nothing about it.

That comes to the return of the function, it returns a pointer to buffer, this is set in the function as static char buffer[40];, but this is never set to anything.  So the caller gets a pointer to a buffer that has never been initialized, the compiler may set the buffer to NULL char‘s but it may not, if not and the buffer will contain garbage characters at run-time, if it does contain NULL char’s the caller will think that the previous font was nothing.

But to top everything the comment of the function indicates that if the name is set the previous name will be returned, well that is not happening here at all.  The writer did not even honor the contract that they wrote.  So the caller could call the function, get a pointer to an array of NULL char’s, think that this is the name of the previous font and then restore the name later on this value; obviously not what they expected as the consumer of the function.

The final indignity is that this function was never called from anywhere, the fix was simple, press [Del] to remove the code, it will now only exist in the annals of history inside git.

Advertisements

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s