You overuse macros, if you have string identifieres that don't change declare them as const char* or as const std::string.

Also just because you rearange code and give variables another names that doesn't mean that it is your code. It is derived from other code but not completely written by you. But with that common stuff it doesn't matter that much. If I would rearange zlib and give variables other names it isn't mine code for example.

In your WndError function use const char* instead of char* because you don't change the error message.

Another thing. Please don't use Dev-C++ it is an old IDE that comes with an old MingW compiler. Switch to Code-Blocks if you want to use MingW or use Visual Studio Express (it is free)

Edit
Another thing. You use globals in your code but you only reference them in you WinMain function. Declare them in there.

Coding style really is a matter of taste and personal preference for the most part, but always aiming for clear, streamlined code so that other coders can make some sense of your work is important too.

So, what I propose below is, I guess, a mixture of the two, although I'm sure others would do things slightly differently.

The code:



in principle is fine, but I would advise that using constant std::string types is better. Also, it's a small point, but constants, along with class and struct definitions are usually best placed within header files.

Asides from usually putting the struct definition (not declaration!)



in a header file, you can give function parameters any name you want. So if you find typing;



a bit cumbersome, then use something like:



However you must use the type 'float' on ALL arguments that ARE floats unfortunately.



Yes, as you stated, WinMain does NOT need to be declared and this is the first time I've seen it declared in someone's code, but also, naming the function parameters in a declaration can be good practise. Usually function declarations are done in header files and if you name the parameters, other coders can get an idea as to what each parameter does. In smart IDEs, as you type in the functions name, a list of possible function prototypes may be displayed, showing you the parameter names. This can sure be a BIG help as your projects get more and more complex with functions being overloaded numerous times!

In the code;



I would initialize Msg. In fact initializing ALL variables defined is usually the best way to go, but I must admit, I don't usually do it for ALL variables myself. However you're taking the address of Msg in the code:



In this case I'm guessing most compilers won't complain because &Msg is treated as a write only [out] argument. But if &Msg was to be read initially without having been initialized, then bad things could happen.

WndClass, hwnd and Done all get written to in the following places;







BEFORE they are read, so I guess no problems there (and I guess initializing Done to false is not strictly needed)

In order to ensure bullet proof code, I would test the return values on virtually all functions that returned error values. I'd probably would not go as far as testing the return on ShowWindow, but I would test CreateWindow for an error code.

The main message loop looks OK to me. Perhaps I would use a DestroyWindow call during the error handling, but the main application will close any live windows automatically on exit. It's not exactly how I would write a message loop myself, but since that's a matter of style I won't digress on it further. The logic of it seems sound.

The last part of WinMain;



has a couple of issues. During the message loop, if all goes well you'll see a live window on your screen. Then, to end the test you'd normally 'x-out' the window (or some other way through the Windows OS). The message loop then receives a WM_QUIT message due to the PostQuitMessage function in the window procedure. GetMessage then returns 0 and the message loop exits.

hwnd is now the handle to a 'dead' window. It's still not null, but it's not valid either. Two ways to handle this is to not even bother to destroy the window yourself (since hopefully it would have already been destroyed) or, use the IsWindow function.

IsWindow takes a window handle and returns true if the handle is still valid and false otherwise. If IsWindow returns true then we can destroy the window if necessary.

So your window destruction code would look like this:



Also, semi-colons (;) are not needed after an "if, else if, else" test in C++.

Unregistering window class is not usually necessary, since user defined window classes will always get unregistered automatically when the application exits, but it's always good to clean up after yourself, so I like your style! [smile]

However, I'm not sure if setting hInstance to null is wise. I'm sure there's no harm in it, but I'm sure hInstance is a WinMain [in] only argument. [in] arguments are usually read, not written to.

The window procedure seems fine to me and unfortunately, has to be written separately from WinMain. Window procedures have to follow some strict guidelines in order for the windows they are responsible for to work correctly.

For your error messaging function;



using std::string won't change your code that much. Here is a version that uses std::string:



It's usually customary to use uppercase names for constants. As you'll notice, to simply get C style strings (i.e. const char * types) from std::string, we simply call the std::string member "c_str ()".

Hope this analysis helped a little!

EDIT: I just noticed one small thing too;



Done should be of type BOOL not bool. BOOL is actually an integer.

[Edited by - CodeStorm on January 24, 2009 9:15:45 AM]