Gavin continues work on the old C++ app he inherited. Today we get a delightful smattering of bad choices.

HRESULT CExAPI::GetPageCacheForFolder( IN  CEntryID *p_cio_Folder, OUT CPageCache **pp_PC )
{
	CEntryID *p_cio_Local = new CEntryID( this, p_cio_Folder );

	switch ( p_cio_Folder->m_uiType )
	{
		case EID_TYPE_EMPTY:
			return S_OK;
			break;
		case EID_TYPE_NORMAL :
			return GetPageCacheForNormalFolder(p_cio_Folder, pp_PC );
			break;
		case EID_TYPE_ADDRESSBOOK :
			return GetPageCacheForABFolder(p_cio_Folder, 0, pp_PC );
			break;
	}

	return S_OK;

	DeleteIfNotNULL( p_cio_Local );
}

We start by converting our pointer to a CEntryID into a pointer to a CEntryID. We construct a new instance along the way, but it's certainly a code smell to do it.

The rest of the code is some awkward branching around the type of folder we're interacting with, but I want to point ou a key thing: each branch of the code returns as soon as it completes, including the final branch, where we return S_OK after the switch. Why that couldn't have been in the default portion of the switch, who knows, but also: if the type field isn't one of these types, is S_OK really what we should be returning?

All of that is minor stuff, though, as our cleanup code which deletes the heap allocated CEntryID is after all the returns, and thus never gets called. Yay memory leaks.

This next one has some questionable logic:

if ( uiMessageType == MT_SENT_SMS )
{
	if ( uiMessageType != MT_SENT_SMS ) POLL_TERMINATE_EVENT_SOK();
	hr = AddAllRecipients( pal, iNumEntries, p_mrl_To, p_mrl_Cc );
	if ( FAILED( hr ) ) return hr;
}
else
{
	if ( uiMessageType != MT_SENT_SMS ) POLL_TERMINATE_EVENT_SOK();
}

If the uiMessageType is MT_SENT_SMS, we check to see if it's not MT_SENT_SMS. Either this logic is stupid, or there's a hidden race condition. And I mean that as a logical "or", so it could easily be both.

Then we try and do an operations, and if FAILED is the result, we… return the result. We don't return the result if it succeeded. Presumably, there's a meaningful return further down the function, but I don't have a huge degree of confidence.

if ( hr == 0x80010104 )
{
	if ( FAILED( RetrieveDetailsUsingOutlook97( p_eioEntryID,
				psPhoneNumber, psEmailAddress, puiEmailIndex,
				psDisplayName, psFirstName, psLastName, puiType ) ) )
	{
		return RetrieveDetailsUsingOutlook97( p_eioEntryID,
				psPhoneNumber, psEmailAddress, puiEmailIndex,
				psDisplayName, psFirstName, psLastName, puiType );
	}
	else
	{
		return S_OK;
	}
}

We start with a wonderful magic number, then we try and RetrieveDetailsUsingOUtlook97, which gives us an idea of how old this code is.

If we fail to RetrieveDetailsUsingOutlook97 we immediately do it again, returning the result. Was this an intentional retry? Given the previous code blocks, I'm not sure- I think it may have been an accident, and they just wanted to get the same error code twice. Or maybe the janky way this talks to Outlook means that the second try usually works. I've certainly seen that happen.

Now, I'm not generally a "there should be a single point of return from every function," person. You should return from wherever it makes the most sense to return, which sometimes is a single point, but sometimes isn't. But this code makes me rethink my policy: the freedom to make choices about how to code leads to people making some really bad choices.

I suppose you can't really build an enforceable code review policy out of "use common sense," since common sense is an oxymoron.

[Advertisement] ProGet’s got you covered with security and access controls on your NuGet feeds. Learn more.