37
\$\begingroup\$

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;
    }
}
\$\endgroup\$
  • 1
    \$\begingroup\$ This makes me think of Haskell's maybe monad, but I don't think that will help you. Still this would totally work with it. \$\endgroup\$ – Theo Belaire Jan 28 '11 at 20:14
  • \$\begingroup\$ @Tyr: Or C++14's optional<T>. :) \$\endgroup\$ – Billy ONeal Jun 28 '13 at 6:29
56
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Would it be possible, or reasonable, to merge the whole code block into one ThrowOnFail(...); including the CComPtr? That would reduce code duplication as well. Not extremely familiar with C++ and CComPtr. \$\endgroup\$ – WernerCD Jan 27 '11 at 13:58
  • \$\begingroup\$ @WernerCD: No, that would not be possible. (Besides, with all those method calls you'd have a 500 character line!) \$\endgroup\$ – Billy ONeal Jan 27 '11 at 15:16
  • 1
    \$\begingroup\$ Err... You know this is really why I shouldn't write code at 3AM :sigh:. \$\endgroup\$ – Billy ONeal Jan 27 '11 at 15:19
  • \$\begingroup\$ Question: Does the error returned in this positively identify where the error came from? \$\endgroup\$ – Hack Saw Jan 27 '11 at 17:48
18
\$\begingroup\$

If I find my self writing the same thing over and over again I usually put it in a functiion 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.

\$\endgroup\$
  • \$\begingroup\$ May I ask you to replace !SUCCEEDED with FAILED? The latter evaluates exactly to the same as !SUCCEEDED. \$\endgroup\$ – sharptooth Jan 28 '11 at 6:35
6
\$\begingroup\$

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.

\$\endgroup\$
  • 1
    \$\begingroup\$ Why an everywhere-reserved name for the local variable? I'd name it _local_D3DCALL (which is reserved in the root namespace scope but not elsewhere) or similar. This macro is a good candidate for calling an (inline) function, passing (a), __FILE__, __LINE__, and #a. \$\endgroup\$ – Fred Nurk Jan 27 '11 at 8:27
  • \$\begingroup\$ No idea, it's been a long time since I wrote it, it's just a sample. \$\endgroup\$ – DeadMG Jan 27 '11 at 11:34
  • \$\begingroup\$ Some macros just for you! -> codereview.stackexchange.com/questions/281/… :) \$\endgroup\$ – Billy ONeal Jan 27 '11 at 15:51
3
\$\begingroup\$

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.

\$\endgroup\$
  • 1
    \$\begingroup\$ This is interesting (therefore I will +1 you), but if you were to ignore the type of errorCheck it would look as if you were ignoring errors. I think I would favor an approach that seems more explicit if you are looking at statements and merely glancing over the types. \$\endgroup\$ – asveikau Jun 14 '11 at 8:54
1
\$\begingroup\$

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 ?

\$\endgroup\$
  • 6
    \$\begingroup\$ The example in the question is very clear that it's the same code over and over. Not functionalising it merely creates a maintenance headache. Should the need arise to handle a specific case differently then you write specific code for it. But to use that as an excuse to not write the general function in the first place is inefficient and IMO ill-advised. \$\endgroup\$ – LRE Jan 27 '11 at 8:24
  • \$\begingroup\$ if this is c++, you can always set the method inline or use a macro with the same functionality \$\endgroup\$ – Pedro Loureiro Jan 27 '11 at 11:50
  • \$\begingroup\$ Don't use a macro. You lose all type safeness. That's the reason for inline functions/methods. \$\endgroup\$ – Mark Loeser Jan 27 '11 at 14:06
  • 2
    \$\begingroup\$ @MarkLoesser - There are times when macros are the right tool for the job. This is one of those times. The FILE and LINE expansions are very helpful in troubleshooting and would be lost if you used an inline function or method. \$\endgroup\$ – Jere.Jones Jan 27 '11 at 18:49
1
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Not true according to this; _com_ptr_t::CreateInstance() seems to return a HRESULT, and has a throw () specification. \$\endgroup\$ – Billy ONeal Jan 27 '11 at 15:54

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.