HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

C++ code to launch a Windows 8 app

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
codelaunchwindowsapp

Problem

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 
#include 
#include 

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

    if (argc  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 !" So I want to come across as a suave sophisticated C++ programmer instead ;)

Solution

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.)

Code Snippets

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

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

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

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

cleanup:
   Cleanup();
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();
      }
   }
}
ComInitialization comInit;

hr = comInit.Initialize();

Context

StackExchange Code Review Q#19080, answer score: 4

Revisions (0)

No revisions yet.