Re: Does this look scary to you?

From:

ajk <foo@nomail.com>

Newsgroups:

microsoft.public.vc.mfc

Date:

Tue, 22 May 2007 22:22:52 +0800

Message-ID:

<5mt553tnpeilssk166u1fg2dtls0ggf1ao@4ax.com>

On Mon, 21 May 2007 14:47:33 -0700, "Leo V" <none@sldkjf.com> wrote:

Okay, I've rewritten the function to hopefully be cleaner.

Please give me your thoughts.

STDMETHODIMP CForm::GetStringValue(LONG lFieldID, /* [out,retval] */ BSTR*
pBstr)
{
   if (!pBstr)
       return E_POINTER;

   _ASSERTE(!(*pBstr) && _T("This should be NULL! It's either not
initialized or is already pointing to allocated memory!"));

   CString csDesc = GetRuntimeText(lFieldID);
   if (csDesc.IsEmpty())
       return E_FAIL; // Is this really a failure?

   try
   {
       *pBstr = csDesc.AllocSysString();
   }
   catch(CMemoryException* pme)
   {
       // NEVER throw an exception over a COM boundary.
       *pBstr = NULL;
       pme->Delete();
   } // catch (CMemoryException*)
   catch(...)
   {
       // NEVER throw an exception over a COM boundary.
       *pBstr = NULL;
   } // catch everything else

   if (*pBstr == NULL)
       return E_OUTOFMEMORY;

   return S_OK;
}

Here is some pointers (IMHO):

I think you should not return NULL in your pBstr, better with empty
string and return S_FALSE. User then can use if (SUCCEED(hr))
SysFreeString(p)

You don't check if FieldID is in range, assuming it is some index then
maybe negative values are not valid?

In general use references to exceptions instead of pointer to
exceptions, that way you can throw the exception again and not worry
who deletes(owns) it.

catch (const CMemoryException &ex)
{

}

The whole function seems more like a property than a method, consider
having it as a property instead.

You swallow the exceptions but return S_OK anyway - have a trace in
your exception handler so that you at least in debug mode notice the
exception. TRACE() or ATLTRACE() whatever your preference/settings
are.

br/ajk