Switch statement use

Question

Should i use this form of switch statement:

  switch(msg)
  {
    case WM_LBUTTONDOWN:
    {
           char szFileName[MAX_PATH];
           HINSTANCE hInstance = GetModuleHandle(NULL);
           GetModuleFileName(hInstance, (LPWCH)szFileName, MAX_PATH);
           MessageBox(hwnd, (LPCWSTR)szFileName, L"This program is:", MB_OK | MB_ICONINFORMATION);
    }
    break;

 case WM_CLOSE:
        DestroyWindow(hwnd);
    break;
    case WM_DESTROY:
        PostQuitMessage(0);
    break;
    default:
        return DefWindowProc(hwnd, msg, wParam, lParam);
  }
  return 0;

or make a function for the first case constant ?

Answer 1

You are missing a break on the first case. Other than that, I would definitely put that code on a separate function.

Answer 2

There's nothing wrong with how you have it, but it's probably cleaner code to call a function so you can keep your functions a reasonable size.

Answer 3

If you are asking if you should turn the code in the first case into a function, then yes, definitely.

Answer 4

Well, it would depend on how many other cases you would have.

Something as small as that, I would say it is not worth it to make it a function, but if your switch statement contains more cases, it will just get ugly, especially if a lot of the cases has multiple lines like that. Putting it into a function would clean it up and make your code look nicer.

Answer 5

One of the most important things I'd say would be consistency. If you create a function for LBUTTONDOWN then create a function for everything. This way there is a predictable pattern for where to find stuff if it breaks.

Related to the topic at hand:

I personally find an if / else if pattern to work better, as it eliminates the problem of a forgotten break:

if (msg == WM_LBUTTONDOWN) {
    // your code here
    return 0;
} else if (msg == WM_DESTROY) {
    PostQuitMessage(0);
    return;
} else if (msg == WM_KEYDOWN) {
    if (wp == VK_F1) {
        DoSomething();
        return;
    }
}
return DefWindowProc(hWnd, msg, wp, lp);

It's really up to you, in the end.

Answer 6

What you going to do when will hadle 20 or 50 window messages?
Maybe it right time for create map - events on functions ( fuctors ) and call them?
Or start to use rule - one message = one function call.

char szFileName[MAX_PATH];
HINSTANCE hInstance = GetModuleHandle(NULL);
GetModuleFileName(hInstance, (LPWCH)szFileName, MAX_PATH);
MessageBox(hwnd, (LPCWSTR)szFileName, L"This program is:", MB_OK | MB_ICONINFORMATION);

Could you explain this strange trick with convetation (LPCWSTR)szFileName. Why you don't use array wchar_t instead casting? - you will have big problem with long paths ( path_length > MAX_PATH / sizeof( wchar_t ) )

One reccomendation - avoid to use casts in general and C-style casts in particulary.

Answer 7

It's fine, but I generally don't like mixing styles and indentations. If I needed to bracket one case I'd probably bracket them all and keep the indentation consistent.

Also bb is right, you should use wchar_t array instead of char in that case.

Answer 8

Also, take a look at message crackers

Answer 9

I would probably declare a map and use functors for every message:

typedef std::map<UINT, boost::function<int (HWND, WPARAM, LPARAM) > > messageFuncs_t;
messageFuncs_t messageFuncs;

Then, when the window class is created, just add a new function for each message:

messageFuncs[WM_LBUTTONDOWN] = &onMouseDownEvent;

... And then implement the message loop thus:

messageFuncs_t::iterator fun = messageFuncs.find(msg);
if(fun != messageFuncs.end())
    return (*fun)(hWnd, wparam, lparam);
else
    return DefWindowProc(hWnd, msg, wp, lp);

... Or whatever works. Then it's easy to add new messages, and the work for each is delegated to a function. Clean, concise, and makes sense.

Answer 10

I'm writing fairly many Win32 message crackers such as these switches.

My rule of thumb is: Wiring up the behavior goes into the switch, behavior into a separate function. That usually means the switch contains the decision whether this command should be handled (e.g. testing for sender ID), and "prettyfying" the parameters.

So in that particular case, a separate function.

The original motivation is that I often end up triggering the behavior in other circumstances (e.g. "when no file name has been specified and the dialog is called with the moon parameter set to full, show the SaveAs dialog immediately").