Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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

share|improve this question
add comment

2 Answers 2

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

share|improve this answer
    
Yeah I think that is the main point I need to attack. The problem is that I want custom error messages for each failure. Here's my newest attempt: gist.github.com/4156732 I have to say, goto seems cleanest. The RAII style makes sense for larger apps but for this small file it increases the line count by a lot. –  Domenic Nov 27 '12 at 20:17
    
@Domenic - Probably not worth fighting religious wars over, but good habits are good habits, large project or not. So I was trying to illustrate the spectrum. If it's C++ and not C I'd say RAII is probably the way to go. One problem with goto is that it is not exception safe. –  asveikau Nov 27 '12 at 20:28
add comment

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

share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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