   C++ Code Structure
  Hello,I've got the basics of a Windows app up and running. You could say it was initially made up of many tutorials pasted together, but I've had a look through the documentation on each part of the code, and added, removed, re-arranged and tidied up bits to the point where I can almost call it my own work and move on to actually making something functional with it....Well, okay, there are still bits which I don't completely understand. I thought I'd post the whole thing up here and ask the opinions of the forum-dwellers.Please look through the code (compiled with Dev-C++) and criticize the style / structure to your heart's content. You'll also find a few commented questions in there.Thanks. =)[updated version of code is in post 3][Edited by - Sappharos on January 24, 2009 7:47:15 AM]
  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)EditAnother thing. You use globals in your code but you only reference them in you WinMain function. Declare them in there.   ;  Ok, thanks for the help. I'll see about making those changes now.Edit: Here's the updated version. // DIRECTIVES#include <windows.h>const char* WndClassName = "WinApp_Main";const char* WndTitle= "Test WinApp";const char* Err_Msg_Inval = "Invalid message.";const char* Err_WndClass_Reg   = "Unable to register window class.";const char* Err_WndClass_UnReg = "Unable to unregister window class.";const char* Err_WndHandle_Rel  = "Unable to release window handle.";// DECLARATIONSstruct SampleStruct/* where it says "float InitX, float InitY, float InitZ", is there any way of   replacing it with something like "float InitX, InitY, InitZ"? It's not   incredibly important, but having to repeatedly define the type can get   annoying. */{    // basic data    float x, y, z;    // constructors    SampleStruct () : x (0), y (0), z (0)    {    };    SampleStruct (float InitX, float InitY, float InitZ) : x (InitX), y (InitY), z (InitZ)    {    };    };/* I know WinMain doesn't really need to be declared, don't worry about that. */int WINAPI WinMain (HINSTANCE, HINSTANCE, LPSTR, int);LRESULT CALLBACK WindowProc (HWND, UINT, WPARAM, LPARAM);void WndError (const char*);// IMPLEMENTATIONint WINAPI WinMain (HINSTANCE hInstance, HINSTANCE hPrevInstance, LPSTR  lpCmdLine, int nCmdShow){    WNDCLASS WndClass;    HWNDhwnd;    MSG Msg;    boolDone = false;    /* How do I make the style NULL without getting Dev-C++ compiler errors? */    WndClass.style    = CS_DBLCLKS;    WndClass.lpfnWndProc   = WindowProc;    WndClass.cbClsExtra    = 0;    WndClass.cbWndExtra    = 0;    WndClass.hInstance= hInstance;    WndClass.hIcon    = LoadIcon (NULL, IDI_APPLICATION);    WndClass.hCursor  = LoadCursor (NULL, IDC_ARROW);    WndClass.hbrBackground = (HBRUSH) GetStockObject (BLACK_BRUSH);    WndClass.lpszMenuName  = NULL;    WndClass.lpszClassName = WndClassName;    if (!RegisterClass (&WndClass))    {   WndError (Err_WndClass_Reg);   return 0;    };   hwnd = CreateWindow (WndClassName,WndTitle,WS_OVERLAPPEDWINDOW,CW_USEDEFAULT,CW_USEDEFAULT,CW_USEDEFAULT,CW_USEDEFAULT,NULL,NULL,hInstance,NULL);    ShowWindow (hwnd, nCmdShow);    // message loop    while ((Done = GetMessage (&Msg, NULL, 0, 0)) != 0)    {   if (Done == -1)   {  WndError (Err_Msg_Inval);    return 0;   }   else   {  TranslateMessage (&Msg);  DispatchMessage (&Msg);   }    };    /* Why does this bring up an error message every time, with no  other apparent problems? Is it because the window has  already been destroyed? */if (hwnd && !DestroyWindow (hwnd)){   WndError (Err_WndHandle_Rel);hwnd = NULL;};if (!UnregisterClass (WndClassName, hInstance)){   WndError (Err_WndClass_UnReg);hInstance = NULL;};    return Msg.wParam;};/* Any way of putting message handling into WinMain? */LRESULT CALLBACK WindowProc (HWND hwnd, UINT uMsg, WPARAM wParam, LPARAM lParam){    switch (uMsg)    {   case WM_DESTROY:  PostQuitMessage (0);  break;   default:  return DefWindowProc (hwnd, uMsg, wParam, lParam);    }    return 0;    };/* Is char* a reasonable type to use here, or is char[] better, or is there  no difference? */void WndError (const char* ErrorText){    MessageBox (NULL, ErrorText, WndTitle, MB_ICONEXCLAMATION);}; [Edited by - Sappharos on January 24, 2009 7:17:39 AM];  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:const char* WndClassName = "WinApp_Main";const char* WndTitle= "Test WinApp";const char* Err_Msg_Inval = "Invalid message.";const char* Err_WndClass_Reg   = "Unable to register window class.";const char* Err_WndClass_UnReg = "Unable to unregister window class.";const char* Err_WndHandle_Rel  = "Unable to release window handle.";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!)// DECLARATIONSstruct SampleStruct/* where it says "float InitX, float InitY, float InitZ", is there any way of   replacing it with something like "float InitX, InitY, InitZ"? It's not   incredibly important, but having to repeatedly define the type can get   annoying. */{    // basic data    float x, y, z;    // constructors    SampleStruct () : x (0), y (0), z (0)    {    };    SampleStruct (float InitX, float InitY, float InitZ) : x (InitX), y (InitY), z (InitZ)    {    };};in a header file, you can give function parameters any name you want. So if you find typing;SampleStruct (float InitX, float InitY, float InitZ) :x (InitX), y (InitY), z (InitZ){};a bit cumbersome, then use something like:SampleStruct (float fx, float fy, float fz) :x (fx), y (fy), z (fz){};However you must use the type 'float' on ALL arguments that ARE floats unfortunately./* I know WinMain doesn't really need to be declared, don't worry about that. */int WINAPI WinMain (HINSTANCE, HINSTANCE, LPSTR, int);LRESULT CALLBACK WindowProc (HWND, UINT, WPARAM, LPARAM);void WndError (const char*);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;// IMPLEMENTATIONint WINAPI WinMain (HINSTANCE hInstance, HINSTANCE hPrevInstance, LPSTR  lpCmdLine, int nCmdShow){    WNDCLASS WndClass;    HWNDhwnd;    MSG Msg;    boolDone = false;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:while ((Done = GetMessage (&Msg, NULL, 0, 0)) != 0)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;WndClass.style    = CS_DBLCLKS;WndClass.lpfnWndProc   = WindowProc;WndClass.cbClsExtra    = 0;WndClass.cbWndExtra    = 0;WndClass.hInstance= hInstance;WndClass.hIcon    = LoadIcon (NULL, IDI_APPLICATION);WndClass.hCursor  = LoadCursor (NULL, IDC_ARROW);WndClass.hbrBackground = (HBRUSH) GetStockObject (BLACK_BRUSH);WndClass.lpszMenuName  = NULL;WndClass.lpszClassName = WndClassName;hwnd = CreateWindow (WndClassName,WndTitle,WS_OVERLAPPEDWINDOW,CW_USEDEFAULT,CW_USEDEFAULT,CW_USEDEFAULT,CW_USEDEFAULT,NULL,NULL,hInstance,NULL);while ((Done = GetMessage (&Msg, NULL, 0, 0)) != 0)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;/* Why does this bring up an error message every time, with no   other apparent problems? Is it because the window has   already been destroyed? */if (hwnd && !DestroyWindow (hwnd)){   WndError (Err_WndHandle_Rel);   hwnd = NULL;};if (!UnregisterClass (WndClassName, hInstance)){   WndError (Err_WndClass_UnReg);   hInstance = NULL;};return Msg.wParam;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:if (IsWindow (hwnd) && !DestroyWindow (hwnd)){   WndError (Err_WndHandle_Rel);   hwnd = NULL;}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;/* Is char* a reasonable type to use here, or is char[] better, or is there  no difference? */void WndError (const char* ErrorText){    MessageBox (NULL, ErrorText, WndTitle, MB_ICONEXCLAMATION);};using std::string won't change your code that much. Here is a version that uses std::string:void WndError (const std::string & ERROR_TEXT){    MessageBox (NULL, ERROR_TEXT.c_str (), WndTitle, MB_ICONEXCLAMATION);};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;bool Done = false;Done should be of type BOOL not bool. BOOL is actually an integer.[Edited by - CodeStorm on January 24, 2009 9:15:45 AM];  That definitely helps, thanks. I'll spend a while looking through this. =)   