C++ code to launch a Windows 8 app

Question

This is my first C++ in many years. It's based on some stuff I found on the internet, including a Microsoft article on a similar topic. But I'm sure there are C++ idioms that might make this better or leaner (my main goal).

#include <ShObjIdl.h>
#include <atlbase.h>
#include <wchar.h>

int wmain(int argc, wchar_t* argv[])
{
    HRESULT hr = S_OK;

    if (argc < 2)
    {
        hr = E_INVALIDARG;
        wprintf(L"Supply an app ID (AppUserModelId) for the application to launch.");
        return hr;
    }

    const wchar_t* appId = argv[1];

    hr = CoInitializeEx(nullptr, COINIT_APARTMENTTHREADED);
    if (FAILED(hr))
    {
        wprintf(L"Error initializing COM");
        return hr;
    }

    CComPtr<IApplicationActivationManager> aam = nullptr;
    hr = CoCreateInstance(CLSID_ApplicationActivationManager, nullptr, CLSCTX_LOCAL_SERVER, IID_PPV_ARGS(&aam));
    if (FAILED(hr))
    {
        wprintf(L"Error creating ApplicationActivationManager");
        CoUninitialize();
        return hr;
    }

    hr = CoAllowSetForegroundWindow(aam, nullptr);
    if (FAILED(hr))
    {
        wprintf(L"Error calling CoAllowSetForegroundWindow");
        CoUninitialize();
        return hr;
    }

    unsigned long pid = 0;
    hr = aam->ActivateApplication(appId, nullptr, AO_NONE, &pid);
    if (FAILED(hr))
    {
        wprintf(L"Error calling ActivateApplication");
        CoUninitialize();
        return hr;
    }

    CoUninitialize();

    return 0;
}

Pretty small and simple, but I want to open-source this. And I definitely know the feeling of looking at JavaScript newbies' code and going "ugh! why didn't they just do <x>!" So I want to come across as a suave sophisticated C++ programmer instead ;)

Answer 1

One piece of feedback is that your error handling code is very repetitive. You are calling CoUninitialize in every failure block. That might seem OK for one cleanup task (though I'd tend to disagree), but once you have N things to clean up on failure (or even successful return), it gets to be a lot of effort and maintenance.

There are a few ways around this that I've seen.

• Have a single block that cleans up (frees any buffers, unintializes COM, whatever). That goes at the end if your function. Make all failure and success paths reach this block.

As an example of how you might do that in COM code, here's the "wrong" way:

hr = Foo();
if (FAILED(hr))
{
   Cleanup();
}

hr = Bar();
if (FAILED(hr))
{
   Cleanup();
}

Cleanup();

There are several choices for the "right" way. One of them would be:

if (SUCCEEDED(hr))
   hr = Foo();
if (SUCCEEDED(hr))
   hr = Bar();

Cleanup();

If you are not religiously opposed to goto (this probably makes more sense in C than C++), this approach is also common:

   hr = Foo();
   if (FAILED(hr))
      goto cleanup;
   hr = Bar();
   if (FAILED(hr))
      goto cleanup;

cleanup:
   Cleanup();

• There are of course more C++ like (rather than C style) ways to prevent cleanup from becoming too much of a hassle. In particular it would probably be a better idea to wrap initialization and de-initialization in the RAII idiom. In this example (COM initialization), you might have a class that (1) initializes COM, (2) tracks that it's been initialized, and (3) in its destructor, deinitializes COM if it has been initialized. Something like this: [nb: I am typing in a web form, it may not be perfect :-)]

Example:

class ComInitialization
{
   bool init;
public:

   ComInitialization() : init(false) {}

   HRESULT Initialize()
   {
      HRESULT hr = S_OK;

      if (!init)
      {
         hr = CoInitializeEx(/* ... */);
         if (SUCCEEDED(hr))
         {
            init = true;
         }
      }

      return hr;
   }

   ~ComInitialization()
   {
      if (init)
      {
         CoUninitialize();
      }
   }
}

With this approach you can have simply:

ComInitialization comInit;

hr = comInit.Initialize();

Then after this block, you can return in any place you like, success or failure, even throw exceptions, and COM will still get uninitialized.

(Notice I didn't initialize COM in a constructor. This allows us to inspect the HRESULT on failure without wrapping it in an exception. I'm sure many would suggest wrapping HRESULTs in exceptions. This is not personally my taste. YMMV.)

Answer 2

I second the RAII recommendation. It's the only same way to do resource management in the presence of exceptions. Even in a small program I would recommend it. For instance, are you certain none of the CComPtr operations you use can throw? Do you want to have to worry about that?

With respect to error handling, my approach, back when I had to deal with COM, was to always #import the relevant .idl file when available. This causes the compiler to autogenerate some wrapper classes that do both reference counting and automatically convert HRESULTs to exceptions. The result was much more concise and readable code (at the expense of fully customized error messages, which was an acceptable tradeoff for my domain).