Code Review Please

[businessman332211]

I just want someone to read over my code below and review it. Tell me their thoughts. This is my first time really messing with Windows API programming. I have been doing C++ for awhile, but first time doing Windows. I have created some basic code that creates a window, and sends some text to it. Can you please review it and tell me if you see ANY issues with my code, or anything that I can do better in the future. I really want to learn this, and taking it one step at a time.

#include <windows.h>

/*  Declare Windows procedure  */
LRESULT CALLBACK WindowProcedure (HWND, UINT, WPARAM, LPARAM);

/*  Make the class name into a global variable  */
char szClassName[ ] = "ComputerInformationDisplayApp";

int WINAPI WinMain (HINSTANCE hThisInstance, HINSTANCE hPrevInstance, LPSTR lpszArgument, int nCmdShow) {
    HWND hwnd;               /* This is the handle for our window */
    MSG messages;            /* Here messages to the application are saved */
    WNDCLASSEX wincl;        /* Data structure for the windowclass */

    /* The Window structure */
    wincl.hInstance = hThisInstance;
    wincl.lpszClassName = szClassName;
    wincl.lpfnWndProc = WindowProcedure;      /* This function is called by windows */
    wincl.style = CS_DBLCLKS;                 /* Catch double-clicks */
    wincl.cbSize = sizeof (WNDCLASSEX);

    /* Use default icon and mouse-pointer */
    wincl.hIcon = LoadIcon (NULL, IDI_APPLICATION);
    wincl.hIconSm = LoadIcon (NULL, IDI_APPLICATION);
    wincl.hCursor = LoadCursor (NULL, IDC_ARROW);
    wincl.lpszMenuName = NULL;                 /* No menu */
    wincl.cbClsExtra = 0;                      /* No extra bytes after the window class */
    wincl.cbWndExtra = 0;                      /* structure or the window instance */
    /* Use Windows's default colour as the background of the window */
    wincl.hbrBackground = (HBRUSH) COLOR_BACKGROUND;

    /* Register the window class, and if it fails quit the program */
    if (!RegisterClassEx (&wincl)) {
        return 0;
    }

    /* The class is registered, let's create the program*/
    hwnd = CreateWindowEx (
           0,                   /* Extended possibilites for variation */
           szClassName,         /* Classname */
           "Computer Information Display",       /* Title Text */
           WS_OVERLAPPEDWINDOW, /* default window */
           CW_USEDEFAULT,       /* Windows decides the position */
           CW_USEDEFAULT,       /* where the window ends up on the screen */
           544,                 /* The programs width */
           375,                 /* and height in pixels */
           HWND_DESKTOP,        /* The window is a child-window to desktop */
           NULL,                /* No menu */
           hThisInstance,       /* Program Instance handler */
           NULL                 /* No Window Creation data */
           );

    /* Make the window visible on the screen */
    ShowWindow (hwnd, nCmdShow);

    /* Run the message loop. It will run until GetMessage() returns 0 */
    while (GetMessage (&messages, NULL, 0, 0)) {
        /* Translate virtual-key messages into character messages */
        TranslateMessage(&messages);
        /* Send message to WindowProcedure */
        DispatchMessage(&messages);
    }

    /* The program return-value is 0 - The value that PostQuitMessage() gave */
    return messages.wParam;
}


/*  This function is called by the Windows function DispatchMessage()  */

LRESULT CALLBACK WindowProcedure (HWND hwnd, UINT message, WPARAM wParam, LPARAM lParam) {

    // Paint Variables
    HDC         hdc ;
    PAINTSTRUCT ps ;
    RECT        rect ;


    /* Handle The Messages */
    switch (message) {
        case WM_PAINT: {
            hdc = BeginPaint (hwnd, &ps) ;
            GetClientRect(hwnd, &rect);
            char *Windowtext = "Testing this out today";
            TextOut (hdc, 5, 5, Windowtext, strlen(Windowtext));
            EndPaint (hwnd, &ps);

            return 0 ;
        }
        case WM_DESTROY:
            PostQuitMessage (0);       /* send a WM_QUIT to the message queue */
            break;
        default:                      /* for messages that we don't deal with */
            return DefWindowProc (hwnd, message, wParam, lParam);
    }

    return 0;
}

Any criticism whatsoever is appreciated. It'll help me learn more in the future.

Also, one question..for the Windowstext char, should I be using String or something else for that instead?

[webJose]

You are using char arrays instead of the correct TCHAR arrays. Either change the data type or explicitly call the ANSI version of the API functions such as TextOutA().

[andywestken]

I don't agree with webJose re the use of TCHAR. It's perfectly OK to decide to build ANSI only. I only use specific calls to the A or W variant of a function when I need to ensure that a bit of code remains unchanged when I swap between ANSI and UNICODE builds. For example, code processing an ASCII text file.

So TCHAR would not be correct here as the code in ANSI (or MBCS) throughout!

Wizard generated code does use TCHAR, LPTSTR, etc and the associated function #defines (_tWinMain rather than WinMain, _tcslen rather than strlen, ...). You might want to change the code to build both ANSI and UNICODE (you'll also need to include tchar.h).

I would not use a string class for the window title, but I would make it a char array like the class name. And the arrays should be made const, since we're talking about C++.

char *Windowtext = "Testing this out today";

would be better as

const char szWindowtext[] = "Testing this out today";

or

const TCHAR szWindowtext[] = _T("Testing this out today");

if you go the ANSI/UNICODE route.

As the code stands it is good C.

To start with, I prefer C++ style comments. But that is more an aesthetic preference!

When I code in C++ I do try and declare variables in a tight a scope as possible. For example, I would move the paint variables inside the WM_PAINT handler:

    /* Handle The Messages */
    switch (message) {
        case WM_PAINT: {
            PAINTSTRUCT ps = {0};
            HDC hdc = BeginPaint (hwnd, &ps) ;
            RECT rect = {0};
            GetClientRect(hwnd, &rect);
            const char szWindowtext[] = "Testing this out today";
            TextOut (hdc, 5, 5, Windowtext, strlen(Windowtext));
            EndPaint (hwnd, &ps);

            return 0 ;
        }

I also initialize all variables.

I would treat WinMain() in a similar manner. Starting with

HWND hwnd = CreateWindowEx( ...

In commerical code I also avoid "magic numbers".

const int windowWidth = 544;
const int windowHeight = 375;

or

SIZE sizeWindow = {544, 375}

(I don't go this far in utility code, but it is a good habit to get into!)

From a WIN32 point of view
- the WNCLASSEX style normally includes the CS_HREDRAW and CS_VREDRAW flags
- you have to add 1 to the color given to hbrBackground to get the one you mean (see MSDN entry for WNDCLASSEX)
- LoadIcon has been superseded by LoadImage, but Wizard generated code still uses LoadIcon. LoadImage allows you to load non-standard sized icons, so something to bear in mind for the future.
- MSG stores only a single message at a time! (hence variable name should be singular)

To facilitate checking against the original code, I initialize the members of a struct in the same order as they are declared.

[andywestken]

And here's your code adjusted based on the notes in my previous message, including switch to TCHAR, etc.

I think that some of your comments would be unnecessary in commercial code, but might get marks in a college exercise. For example, the comments for window style, window title, ...

Note that I've also added a guard against window creation failure.

#define WIN32_LEAN_AND_MEAN // Exclude rarely-used stuff from Windows headers
#include <windows.h>
#include <tchar.h>

LRESULT CALLBACK WindowProcedure (HWND, UINT, WPARAM, LPARAM);

const TCHAR szClassName  [ ] = _T("ComputerInformationDisplayApp");
const TCHAR szWindowTitle[ ] = _T("Computer Information Display");

const SIZE sizeWindow = {544, 375};

int WINAPI _tWinMain (HINSTANCE hThisInstance, HINSTANCE hPrevInstance, LPTSTR lpszArgument, int nCmdShow) {
    // The Window structure
    WNDCLASSEX wincl;
    wincl.cbSize        = sizeof (WNDCLASSEX);
    wincl.style         = CS_HREDRAW | CS_VREDRAW | CS_DBLCLKS; // Catch double-clicks
    wincl.lpfnWndProc   = WindowProcedure; // This function is called by windows
    wincl.cbClsExtra    = 0;               // No extra bytes after the window class
    wincl.cbWndExtra    = 0;               // structure or the window instance
    wincl.hInstance     = hThisInstance;
    // Use default icon and mouse-pointer
    wincl.hIcon         = LoadIcon (NULL, IDI_APPLICATION);
    wincl.hCursor       = LoadCursor (NULL, IDC_ARROW);
    // Use Windows's default colour as the background of the window
    wincl.hbrBackground = (HBRUSH)(COLOR_BACKGROUND + 1);
    wincl.lpszMenuName  = NULL;            // No menu
    wincl.lpszClassName = szClassName;
    // As for large icon
    wincl.hIconSm       = LoadIcon (NULL, IDI_APPLICATION);

    // Register the window class, and if it fails quit the program
    if (!RegisterClassEx (&wincl)) {
        return 0;
    }

    // The class is registered, let's create the program*/
    HWND hwnd = CreateWindowEx (
                0,                   // No extended style
                szClassName,         // Class name
                szWindowTitle,       // Title Text
                WS_OVERLAPPEDWINDOW, // default window style
                CW_USEDEFAULT,       // Windows decides the position
                CW_USEDEFAULT,       // where the window ends up on the screen
                sizeWindow.cx,       // The programs main window width
                sizeWindow.cy,       // and height in pixels
                HWND_DESKTOP,        // The window is a child-window to desktop
                NULL,                // No menu
                hThisInstance,       // Program Instance handler
                NULL                 // No Window Creation data
                );

    if (!hwnd) {
        return 0;
    }

    // Make the window visible on the screen
    ShowWindow (hwnd, nCmdShow);

    // Run the message loop. It will run until GetMessage() returns 0
    MSG msg = {0};
    while (GetMessage (&msg, NULL, 0, 0)) {
        // Translate virtual-key messages into character messages
        TranslateMessage(&msg);
        // Send message to WindowProcedure
        DispatchMessage(&msg);
    }

    // The program return-value is 0 - The value that PostQuitMessage() gave
    return msg.wParam;
}

LRESULT CALLBACK WindowProcedure (HWND hwnd, UINT message, WPARAM wParam, LPARAM lParam) {

    switch (message) {
        case WM_PAINT: {
            PAINTSTRUCT ps = {0};
            HDC hdc = BeginPaint (hwnd, &ps) ;
            RECT rect = {0};
            GetClientRect(hwnd, &rect);
            const TCHAR szWindowText[] = _T("Testing this out today");
            POINT pointText = {5, 5};
            TextOut (hdc, pointText.x, pointText.y, szWindowText, _tcslen(szWindowText));
            EndPaint (hwnd, &ps);
            return 0 ;
        }
        case WM_DESTROY:
            PostQuitMessage (0);
            break;
        default:
            return DefWindowProc (hwnd, message, wParam, lParam);
    }

    return 0;
}

[businessman332211]

OK cool. Wasn't expecting that much feedback..that has spawned a few questions that I have no idea about.

First off my goal is to code in C++, not C-Style. I haven't worked with C before, although I probably will in the future if I find a need.

For the commenting, I am just use to PHP. So I use // for single line comments, and /* whatever */ for multi line comments. Just habit, might as well stick with it. Since it makes sense from a code beauty view point.

Question: What is the difference between "ANSI" and the other type you were talking about. I do not understand that. The only thing I really know/understand about Ansi at all is it has something to do with incoding. So what do I need to know about ANSI when dealing with C++, what is the difference?

For the "code" what I did is I performed a template using Codeblocks, and broke it up so I understood it. Then I went ahead and used various bits of code to add the window write functionality. I am still learning overall how the different parts of window creation work.

Question: What is the difference between the different (3 different) types of "TextOut" functions? Is there ever a reason to use the others?

Thanks again for all the help/feedback.

[businessman332211]

One more qustion.

char *Windowtext = "Testing this out today";

would be better as

const char szWindowtext[] = "Testing this out today";

or

const TCHAR szWindowtext[] = _T("Testing this out today");

My only question is Why?
I just want to understand "Why" and the differences between the two.

[businessman332211]

Also, what are "Magic Numbers". I did some Googling and it gives different definitions. Not sure what you mean by that?

[businessman332211]

According to this: http://users.csc.calpoly.edu/~bfriesen/software/builds.html
You would never need to do Unicode unless you were working for ONLY Windows NT. I don't even have that system, so I always want it to be Ansi then right? I would never have a reason for Unicode in my apps if that is the case, since I want to support all possible versions of Windows. Is this correct, also sorry about so many posts.

[modoran]

I would never have a reason for Unicode in my apps if that is the case, since I want to support all possible versions of Windows.

You haven't heard about MSLU ? (Microsoft Layer for Unicode). This brings Unicode API's to every windows versions.

[businessman332211]

No I have never heard of that before. But that just leaves more questions. For general apps that I am developing for...should I try to develop in Ansi or Unicode? Do I need to use different ones for different things? What all does it have to do with, just the way the functions are called? Or is it much more far reaching?

[modoran]

Well, there are functions in win32api that are unicode only, like GetCommandLineW() and others. Internally, windows kernel uses just Unicode. ANSI versions just convert from Ansi to Unicode internally, using memory and CPU power.
So, for new applications, you are recommended to use just Unicode. It is very likely that Ansi APIs will be removed in future versions of windows.

[businessman332211]

OK, that helps me understand better. So overall..when writing code what should I do to try and ensure that my program is written in Unicode? Is that going to affect EVERYTHING...the way I call functions, the way I handle variables? What exactly do I need to be aware of when developing applications to try to support entire Unicode?

Thanks again for all the feedback.

[modoran]

Just add this before including <windows.h>

#define _UNICODE
#define UNICODE 

And when use string constants use "L" prefix and wchar_t type, instead of char, like this:
wchar_t myString[] = L"This is unicode text: €ășțăî";

[businessman332211]

OK and your 100% sure that if I create via Unicode that it'll work on all windows systems that ANSI will work on, out of the box? I mean the owners of the computers don't have to download anything special to get support...so this'll work on any computer that Ansi?

[modoran]

On windows 2000 or higher, this is 100% sure. On earlier versions, users must have installed MSLU dll (it is a free download from Microsoft web site.)

[DSLq5Di1]

The choice between ASCII and Unicode would depend on, if you plan to implement support for other languages? Unicode (2 bytes) is required to represent a character in some languages, chinese for example.

char *Windowtext = "Testing this out today";

More accurately.. const is implicit here, I believe this is a deprecated form and you will be warned, assuming the warning level of your compiler is set appropriately.

const char szWindowtext[] = "Testing this out today";

szWindowText decays to a const char pointer of the first element. Functionally there is no difference between your first two lines.

const TCHAR szWindowtext[] = _T("Testing this out today");

TCHAR macros are provided to ease the transition of switching between differing character sets. If you have a look at the definitions in the windows headers you can see what is going on here,

#ifdef  UNICODE // This will be defined depending on your project settings.
typedef wchar_t TCHAR
#define _T(x)  L ## x // The preprocessor replaces _T("Example") with L"Example"
#define MessageBox  MessageBoxW
#else
typedef char TCHAR
#define _T(x)  x
#define MessageBox  MessageBoxA // ASCII version of MessageBox
#endif 

Similarly there are #defines for almost every Windows API and many of the CRT functions have TCHAR alternatives.

[andywestken]

Regarding ANSI entry points.

If you have no need to support Windows 95/98/ME, you can forget about them. If you do need to, as mordoran said, you can use the MSLU.

The Windows NT family (Windows NT 3.1/3.5.x, Windows 4.0, Windows 2000, Windows XP, Windows Vista, Windows 7) has always been UNICODE internally. As modoran also said, all ANSI entry points call through to their UNICODE equivalents.

And UNICODE makes mutli-language support a lot easier.

The Windows 95 family handled characters outside of the "normal" (i.e. American) range by multi-byte character sets. This involved coding with the _mbc- versions of the string functions (the _tcs- function map to the _mbc- ones when _MBCS is defined)

Luckily I haven't needed to touch one single _mbclen this millenium!

[andywestken]

Regarding

const char* szWindowtext = "Testing this out today";

versus

const char szWindowtext[] = "Testing this out today";

Is the first case a const array is created, plus a pointer which is initialized to point at the array.

In the latter case there is just the array. The address of its first element is passed directly to a function.

It's perfectly acceptable to walk the string with the pointer, so the extra variable cannot be compiled out.

But I would expect the following to compile to the same as the array form.

char const * const szWindowtext = "Testing this out today";

Andy

P.S. Sadly, VC++ 2008 does not warn, at /W4, that char* sz = "Value"; is stupid. It just access violates if you're stupid enough to try and change an element. :-(

[andywestken]

By "magic number" I mean a literal that has no meaning in isolation. If you have to add a comment to explain what a value is, then you should be using a constant which uses that comment as its name.

But not to totally ludicrous extremes!

diameter = radius * 2 * pi; :-)

diameter = radius * two * pi; :-(

It's ok to use a number when it is just a number!

Accoding to the "self documenting code" school, you shouldn't need comment except when you're explaining how something is done. What a function and member do should be clear from their name. And this follows through to literals.

Andy

P.S. http://stackoverflow.com/questions/209015/self-documenting-code

[andywestken]

Regarding encoding.

Yes, ANSI is just one encoding. And UNICODE another.

Note that the Windows concept of UNICODE was a bit ropey in the early days. I've seen people whinging about it, but as I work with Windows it's difficult for me to see the outside view.

Strictly speaking, Windows UNICODE refers to UTF-16 (or a subset of). But newer versions of Windows now support other forms : UTF-7, UTF-8, UTF-16 (more completely), and UTF-32.

Internally I would expect it's all UTF-16 -- that is, 2 bytes per characters.

From what I can gather, UTF-8 is popular elsewhere, esp. on the Internet. This is a mutli-byte character set.