Resolving a link

Question

How can I clean this up?

std::wstring LinkResolve::ResolveLink( const std::wstring& source ) const
{
    HRESULT errorCheck;
 wchar_t linkTarget[MAX_PATH];
 wchar_t expandedTarget[MAX_PATH];
 wchar_t arguments[INFOTIPSIZE];
    ATL::CComPtr<IPersistFile> ipf;
    errorCheck = ipf.CoCreateInstance(CLSID_ShellLink, 0, CLSCTX_INPROC_SERVER);
    if (!SUCCEEDED(errorCheck))
    {
        throw _com_error(errorCheck);
    }
    errorCheck = ipf->Load(source.c_str(), 0);
    ATL::CComPtr<IShellLink> shellLink;
    errorCheck = ipf->QueryInterface(&shellLink);
    if (!SUCCEEDED(errorCheck))
    {
        throw _com_error(errorCheck);
    }
    errorCheck = shellLink->Resolve(0, SLR_NO_UI);
    if (!SUCCEEDED(errorCheck))
    {
        throw _com_error(errorCheck);
    }
    errorCheck = shellLink->GetPath(linkTarget, MAX_PATH, 0, SLGP_RAWPATH);
    if (!SUCCEEDED(errorCheck))
    {
        throw _com_error(errorCheck);
    }
    ExpandEnvironmentStringsW(linkTarget, expandedTarget, MAX_PATH);
    errorCheck = shellLink->GetArguments(arguments, INFOTIPSIZE);
    if (SUCCEEDED(errorCheck))
    {
        return std::wstring(expandedTarget) + L" " + arguments;
    }
    else
    {
        return expandedTarget;
    }
}

Answer 1

Personally, I'd write a simple function:

void ThrowOnFail( HRESULT hrcode )
{
    if (FAILED(hrcode))
        throw _com_error(hrcode);
}

Then the function calls become:

ThrowOnFail( ipf.CoCreateInstance(CLSID_ShellLink, 0, CLSCTX_INPROC_SERVER) );
ThrowOnFail( ipf->Load(source.c_str(), 0) );
ATL::CComPtr<IShellLink> shellLink;
ThrowOnFail( ipf->QueryInterface(&shellLink) );
ThrowOnFail( shellLink->Resolve(0, SLR_NO_UI) );
ThrowOnFail( shellLink->GetPath(linkTarget, MAX_PATH, 0, SLGP_RAWPATH) );

Incidentally, you missed a check for errorCheck after Load. This becomes easier to spot with a check function.

Answer 2

If I find my self writing the same thing over and over again I usually put it in a function somewhere. Even if that function in your case is as simple as this:

void check(HRESULT result) {
    if (FAILED(result)) {
        throw _com_error(result);
    }
}

I think the code looks fairly straight forward if you reuse your error check code. I'm not familiar with the API that you are using, so I can't comment on if there is another way to use it that might result in cleaner code.

Answer 3

At least when using DirectX, I use a macro.

#define D3DCALL(a) { auto __ = a; if (FAILED(__)) DXTrace(__FILE__, __LINE__, __, WIDEN(#a), TRUE); }

You could get fancier and use a type with an operator=(HRESULT) to make the check.

Answer 4

I remember seeing something like this before:

class XHR
{
public:
   XHR() {};
   ~XHR() {};

   operator HRESULT() { return hr };

   HRESULT& operator =(const HRESULT& rhs)
   {
       hr = rhs;
       if (FAILED(hr)
          throw _com_error(hr);
   }

private:
   HRESULT hr;
}

Then you can write:

std::wstring LinkResolve::ResolveLink( const std::wstring& source ) const
{
    XHR errorCheck;   // instead of HRESULT errorCheck

    wchar_t linkTarget[MAX_PATH];
    wchar_t expandedTarget[MAX_PATH];
    wchar_t arguments[INFOTIPSIZE];
    ATL::CComPtr<IPersistFile> ipf;


    errorCheck = ipf.CoCreateInstance(CLSID_ShellLink, 0, CLSCTX_INPROC_SERVER);
    errorCheck = ipf->Load(source.c_str(), 0);

    ATL::CComPtr<IShellLink> shellLink;

    errorCheck = ipf->QueryInterface(&shellLink);
    errorCheck = shellLink->Resolve(0, SLR_NO_UI);
    errorCheck = shellLink->GetPath(linkTarget, MAX_PATH, 0, SLGP_RAWPATH);

    ExpandEnvironmentStringsW(linkTarget, expandedTarget, MAX_PATH);

    try {
       errorCheck = shellLink->GetArguments(arguments, INFOTIPSIZE);
       return std::wstring(expandedTarget) + L" " + arguments;
    }
    catch (const XHR&)
    {
        return expandedTarget;
    }
}

So anytime an HRESULT that indicates failure, it will automatically convert it into a _com_error for you, and throw it.

Answer 5

Error checks are not all the same. sometimes you act upon special returned HResults. sometimes there is an ELSE. sometime you want to log the error,sometime you don't..

Also - although highly unlikely to be relevant in the com/atl world - calling a function has it's performance costs.

so I prefer using if after the call, rather than calling a function. How much do you save ? typing 10 chars ?

Answer 6

I think you can do much of the same by using Compiler com support here is an example.

#import "CLSID:lnkfile" //use the clsid of the ShellLink class.

IPersistFilePtr ptr = IPersistFilePtr.CreateInstance(...);

_com_ptr_t::CreateInstance() will throw an exception (of type _com_error if the call fails)

All other ifs in your code can be replaced by using the smart pointers generated by #import. I know I am a little skimpy on details but it has been a long time since I have touched COM.