5
\$\begingroup\$

I've implemented Conway's Game of Life using the raw Windows API and GDI as an exercise. I'm wondering if there is room for improvement, though.

Beware, large amount of code following:

CGOL.h:

#include <Windows.h>
#include <windowsx.h>
#include <stdlib.h>
#include <time.h>
#include <sal.h>

#define IDT_TIMER1 1001
#define EXTERN extern // OCD with the caps stuff
#define GRIDSIZE 80
#define CELLSIZE 10

EXTERN CONST WCHAR g_wszClassName[];
EXTERN INT g_aGrid[GRIDSIZE][GRIDSIZE];
EXTERN INT g_aUpdate[GRIDSIZE][GRIDSIZE];
EXTERN HDC g_hDC;
EXTERN HBRUSH g_hbrCell;
EXTERN HBRUSH g_hbrBackground;

VOID WINAPI OnClose(_In_ HWND hWnd);
VOID WINAPI OnDestroy(_In_ HWND hWnd);
BOOL WINAPI OnCreate(_In_ HWND hWnd, _In_ LPCREATESTRUCTW lpCreateStruct);
ATOM WINAPI RegisterWCEX(_In_ HINSTANCE hInstance);
VOID CALLBACK TimerProc(_In_ HWND hWnd, _In_ UINT uMsg, _In_ UINT_PTR idEvent, _In_ DWORD dwTime);
LRESULT CALLBACK WindowProc(_In_ HWND hWnd, _In_ UINT Msg, _In_ WPARAM wParam, _In_ LPARAM lParam);
INT APIENTRY wWinMain(_In_ HINSTANCE hInstance, _In_ HINSTANCE hPrevInstance, _In_z_ LPWSTR lpCmdLine, _In_ INT nShowCmd);

onclose.c:

#include "CGOL.h"

VOID WINAPI OnClose(
    _In_ HWND hWnd
)
{
    KillTimer(hWnd, IDT_TIMER1);
    DestroyWindow(hWnd);
}

oncreate.c:

#include "CGOL.h"

HBRUSH g_hbrCell;
HBRUSH g_hbrBackground;
INT g_aGrid[GRIDSIZE][GRIDSIZE];
INT g_aUpdate[GRIDSIZE][GRIDSIZE];

BOOL WINAPI OnCreate(
    _In_ HWND hWnd, 
    _In_ LPCREATESTRUCTW lpCreateStruct
)
{
    int x, y;
    g_hbrCell = CreateSolidBrush(RGB(0, 255, 0));
    g_hbrBackground = CreateSolidBrush(RGB(0, 0, 0));
    srand((UINT)time(NULL));
    ZeroMemory(g_aGrid, sizeof(INT) * GRIDSIZE * GRIDSIZE);
    ZeroMemory(g_aUpdate, sizeof(INT) * GRIDSIZE * GRIDSIZE);
    for (y = 0; y < GRIDSIZE; y++)
    {
        for (x = 0; x < GRIDSIZE; x++)
        {
            g_aGrid[y][x] = ((rand()%2)==0);
        }
    }
    for (y = 0; y < GRIDSIZE; y++)
        for (x = 0; x < GRIDSIZE; x++)
            g_aUpdate[y][x] = g_aGrid[y][x];

    g_hDC = GetDC(hWnd);
    SetTimer(hWnd, IDT_TIMER1, 100, TimerProc);
    return TRUE;
}

ondestroy.c:

#include "CGOL.h"

VOID WINAPI OnDestroy(
    _In_ HWND hWnd
)
{
    PostQuitMessage(0);
}

registerwcex.c:

#include "CGOL.h"

ATOM WINAPI RegisterWCEX(
    _In_ HINSTANCE hInstance
)
{
    WNDCLASSEXW wcex;
    ZeroMemory(&wcex, sizeof(WNDCLASSEXW));

    wcex.cbSize = sizeof(WNDCLASSEXW);
    wcex.hbrBackground = (HBRUSH)(COLOR_WINDOW + 1);
    wcex.hCursor = LoadCursorW(NULL, IDC_ARROW);
    wcex.hIcon = LoadIconW(NULL, IDI_APPLICATION);
    wcex.hIconSm = LoadIconW(NULL, IDI_APPLICATION);
    wcex.hInstance = hInstance;
    wcex.lpfnWndProc = WindowProc;
    wcex.lpszClassName = g_wszClassName;

    return RegisterClassExW(&wcex);
}

timerproc.c:

#include "CGOL.h"

HDC g_hDC;

VOID CALLBACK TimerProc(
    _In_ HWND hWnd, 
    _In_ UINT uMsg, 
    _In_ UINT_PTR idEvent, 
    _In_ DWORD dwTime
)
{
    int x, y, nAdj = 0;
    SelectObject(g_hDC, g_hbrBackground);
    Rectangle(g_hDC, 0, 0, GRIDSIZE * CELLSIZE, GRIDSIZE * CELLSIZE);
    SelectObject(g_hDC, g_hbrCell);
    for (y = 0; y < GRIDSIZE; y++)
        for (x = 0; x < GRIDSIZE; x++)
            if (g_aUpdate[y][x] == 1)
                Rectangle(g_hDC, x * CELLSIZE, y * CELLSIZE, x * CELLSIZE + CELLSIZE, y * CELLSIZE + CELLSIZE);

    for (y = 0; y < GRIDSIZE; y++)
        for (x = 0; x < GRIDSIZE; x++)
            g_aGrid[y][x] = g_aUpdate[y][x];


    for (y = 1; y < GRIDSIZE-1; y++)
    {
        for (x = 1; x < GRIDSIZE-1; x++)
        {
            if (g_aGrid[y][x - 1] == 1)
                nAdj++;
            if (g_aGrid[y - 1][x - 1] == 1)
                nAdj++;
            if (g_aGrid[y - 1][x] == 1)
                nAdj++;
            if (g_aGrid[y - 1][x + 1] == 1)
                nAdj++;
            if (g_aGrid[y][x + 1] == 1)
                nAdj++;
            if (g_aGrid[y + 1][x + 1] == 1)
                nAdj++;
            if (g_aGrid[y + 1][x] == 1)
                nAdj++;
            if (g_aGrid[y + 1][x - 1] == 1)
                nAdj++;
            if (g_aGrid[y][x] == 0 && nAdj == 3)
                g_aUpdate[y][x] = 1;
            if (g_aGrid[y][x] == 1)
            {
                if (nAdj > 3 || nAdj < 2)
                    g_aUpdate[y][x] = 0;
            }
            nAdj = 0;
        }
    }
    nAdj = 0;

    // EDGES
    // LEFT VERTICAL EDGE
    for (y = 1; y < GRIDSIZE-1; y++)
    {
        nAdj = 0;
        if (g_aGrid[y + 1][0] == 1)
            nAdj++;
        if (g_aGrid[y][1] == 1)
            nAdj++;
        if (g_aGrid[y - 1][0] == 1)
            nAdj++;
        if (g_aGrid[y + 1][1] == 1)
            nAdj++;
        if (g_aGrid[y - 1][1] == 1)
            nAdj++;
        if (nAdj > 3 || nAdj < 2)
            g_aUpdate[y][0] = 0;
        if (g_aGrid[y][0] == 0 && nAdj == 3)
            g_aUpdate[y][0] = 1;

    }
    // RIGHT VERTICAL EDGE
    for (y = 1; y < GRIDSIZE-1; y++)
    {
        nAdj = 0;
        if (g_aGrid[y + 1][GRIDSIZE-1] == 1)
            nAdj++;
        if (g_aGrid[y][GRIDSIZE-2] == 1)
            nAdj++;
        if (g_aGrid[y - 1][GRIDSIZE-1] == 1)
            nAdj++;
        if (g_aGrid[y + 1][GRIDSIZE-2] == 1)
            nAdj++;
        if (g_aGrid[y - 1][GRIDSIZE-2] == 1)
            nAdj++;
        if (nAdj > 3 || nAdj < 2)
            g_aUpdate[y][GRIDSIZE-1] = 0;
        if (g_aGrid[y][GRIDSIZE-1] == 0 && nAdj == 3)
            g_aUpdate[y][GRIDSIZE-1] = 1;

    }

    // TOP HORIZONTAL EDGE
    for (x = 1; x < GRIDSIZE-1; x++)
    {
        nAdj = 0;
        if (g_aGrid[0][x + 1] == 1)
            nAdj++;
        if (g_aGrid[1][x] == 1)
            nAdj++;
        if (g_aGrid[0][x - 1] == 1)
            nAdj++;
        if (g_aGrid[1][x + 1] == 1)
            nAdj++;
        if (g_aGrid[1][x - 1] == 1)
            nAdj++;
        if (nAdj > 3 || nAdj < 2)
            g_aUpdate[0][x] = 0;
        if (g_aGrid[0][x] == 0 && nAdj == 3)
            g_aUpdate[0][x] = 1;
    }

    // BOTTOM HORIZONTAL EDGE
    for (x = 1; x < GRIDSIZE-1; x++)
    {
        nAdj = 0;
        if (g_aGrid[GRIDSIZE-1][x + 1] == 1)
            nAdj++;
        if (g_aGrid[GRIDSIZE-2][x] == 1)
            nAdj++;
        if (g_aGrid[GRIDSIZE-1][x - 1] == 1)
            nAdj++;
        if (g_aGrid[GRIDSIZE-2][x + 1] == 1)
            nAdj++;
        if (g_aGrid[GRIDSIZE-2][x - 1] == 1)
            nAdj++;
        if (nAdj > 3 || nAdj < 2)
            g_aUpdate[GRIDSIZE-1][x] = 0;
        if (g_aGrid[GRIDSIZE-1][x] == 0 && nAdj == 3)
            g_aUpdate[GRIDSIZE-1][x] = 1;
    }

    nAdj = 0;

    // CORNERS
    if (g_aGrid[0][0] == 1)
    {
        if (g_aGrid[0][1] == 1)
            nAdj++;
        if (g_aGrid[1][0] == 1)
            nAdj++;
        if (g_aGrid[1][1] == 1)
            nAdj++;
        if (nAdj < 2)
            g_aUpdate[0][0] = 0;
    }
    nAdj = 0;

    if (g_aGrid[0][GRIDSIZE-1] == 1)
    {
        if (g_aGrid[1][GRIDSIZE-1] == 1)
            nAdj++;
        if (g_aGrid[0][GRIDSIZE-2] == 1)
            nAdj++;
        if (g_aGrid[1][GRIDSIZE-2] == 1)
            nAdj++;
        if (nAdj < 2)
            g_aUpdate[0][GRIDSIZE-1] = 0;
    }
    nAdj = 0;

    if (g_aGrid[GRIDSIZE-1][0] == 1)
    {
        if (g_aGrid[GRIDSIZE-1][1] == 1)
            nAdj++;
        if (g_aGrid[GRIDSIZE-2][0] == 1)
            nAdj++;
        if (g_aGrid[GRIDSIZE-2][1] == 1)
            nAdj++;
        if (nAdj < 2)
            g_aUpdate[GRIDSIZE-1][0] = 0;
    }
    nAdj = 0;
    if (g_aGrid[GRIDSIZE-1][GRIDSIZE-1] == 1)
    {
        if (g_aUpdate[GRIDSIZE-1][GRIDSIZE-2] == 1)
            nAdj++;
        if (g_aUpdate[GRIDSIZE-2][GRIDSIZE-1] == 1)
            nAdj++;
        if (g_aUpdate[GRIDSIZE-2][GRIDSIZE-2] == 1)
            nAdj++;
        if (nAdj < 2)
            g_aUpdate[GRIDSIZE-1][GRIDSIZE-1] = 0;
    }
    nAdj = 0;
}

windowproc.c:

#include "CGOL.h"

LRESULT CALLBACK WindowProc(
    _In_ HWND hWnd, 
    _In_ UINT Msg, 
    _In_ WPARAM wParam, 
    _In_ LPARAM lParam
)
{
    switch (Msg)
    {
        HANDLE_MSG(hWnd, WM_CREATE, OnCreate);
        HANDLE_MSG(hWnd, WM_CLOSE, OnClose);
        HANDLE_MSG(hWnd, WM_DESTROY, OnDestroy);
    default:
        return DefWindowProcW(hWnd, Msg, wParam, lParam);
    }
    return 0;
}

wwinmain.c:

#include "CGOL.h"

CONST WCHAR g_wszClassName[] = L"CGOL_GovindParmar";

INT APIENTRY wWinMain(
    _In_ HINSTANCE hInstance, 
    _In_ HINSTANCE hPrevInstance, 
    _In_z_ LPWSTR lpCmdLine, 
    _In_ INT nShowCmd
)
{
    HWND hWnd; 
    MSG Msg; 

    if (RegisterWCEX(hInstance)==(ATOM)0) 
    {
        MessageBoxW(NULL, L"Window registration failed", L"Error", MB_ICONSTOP | MB_OK);
        return 0;
    }

    hWnd = CreateWindowExW(WS_EX_OVERLAPPEDWINDOW, g_wszClassName, L"Conway\'s Game of Life", WS_VISIBLE | WS_SYSMENU, 0, 0, GRIDSIZE * CELLSIZE, GRIDSIZE * CELLSIZE, NULL, NULL, hInstance, NULL);

    if (hWnd == NULL) 
    {
        MessageBoxW(NULL, L"Window creation failed", L"Error", MB_ICONSTOP | MB_OK);
        return 0;
    }

    ShowWindow(hWnd, SW_SHOW);
    UpdateWindow(hWnd);

    while (GetMessageW(&Msg, NULL, 0, 0) > 0) 
    { 
        TranslateMessage(&Msg); 
        DispatchMessageW(&Msg); 
    }
    return Msg.wParam;
}

My questions are:

  1. You will notice in the code for TimerProc that I first process all the inner cells of the grid that are not an edge or corner of the grid, then the edges, then finally the corners. This is because I don't want undefined behavior when I access all the adjacent elements of a cell but there isn't anything adjacent in the array, like on an edge when there isn't anything above/below/left/right of the cell. Is there another way to do this so that TimerProc doesn't have the amount of code that it does?

  2. I use the array g_aUpdate to hold the next generation of cells then update g_aGrid as well as the drawing on the window based on its state every timer tick. Is there a way I can avoid the excessive copying and still produce the same output?

  3. Are there any GDI leaks in this code? I don't think there are.

  4. General code review stuff, variable names, indentation, code style, etc.

Improve this question
\$\endgroup\$

1 Answer 1

Reset to default
5
\$\begingroup\$

Regarding 1.: Sure, you can just add a border to your field. That's trading space for code-simplicity, which is distinct from efficiency.

Regarding 2.: Sure, there's no reason why you cannot just designate one of the fields as the active one. Needs no more additional space than a boolean, or a pointer.

Regarding 3.: You have some permanently allocated GDI-resources, but nothing continually leaking.

  1. I would suggest not making each and every function its own TU. Doing so makes navigation cumbersome, especially as the number of functions increases.

  2. I suggest signalling failure of registering/creating your main-window by exiting with EXIT_FAILURE instead of 0.

  3. Redrawing the field on timer firing and then recalculating the field is curiously backwards.

  4. Anyway, the timer function should only invalidate the client rectangle and let WM_PAINT do the graphics work. As a bonus, the initial drawing and redrawing as needed works.

  5. Correct me if I'm wrong, but cells can only be alive or dead, so 1 or 0. Why don't you just add up the neighbors liveness? And why an INT for each cell?

  6. Also, as you proceed from start of a row to the end, why do you always start over instead of taking the last count and adjusting it?

  7. Why don't you calculate a single value from a neighbourhood (2 * live_cells_in_3x3 + is_live which is less than 20) and then use that to look up the result.

  8. You only need the windowclassname for registering the windowclass. In main() you can use the ATOM returned instead. Eliminating globals is good.

  9. It's a bit of a guessing-game in which TU each global is defined. Maybe a symptom of having too many files.

  10. Static memory starts out zeroed unless explicitly otherwise initialized.

Modified function to step the game (Using C's poor-man's lambda):

#define HEIGHT 90
#define WIDTH 90

void cgol_step(
    _In_reads_(HEIGHT * WIDTH) const char* src,
    _Out_writes_all_(HEIGHT * WIDTH) char* dest
) {
    #define TRANSITION() (dest[i] = (1 & ( 0x2c0UL >> ((a + b + c) * 2 + src[i]))))

    #define DO_LINE(first, update) do { \
        int i = (first) + WIDTH, a, b = 0, c; \
        c = (update); \
        while(--i > (first)) { \
            a = b; \
            b = c; \
            c = (update); \
            TRANSITION(); \
        } \
        a = 0; \
        TRANSITION(); \
    } while(0)

    DO_LINE(0, src[i - 1] + src[i + WIDTH - 1]);
    for(int j = WIDTH; j < WIDTH * (HEIGHT - 1); j += WIDTH)
        DO_LINE(j, src[i - WIDTH - 1] + src[i - 1] + src[i + WIDTH - 1]);
    DO_LINE(WIDTH * (HEIGHT - 1), src[i - WIDTH - 1] + src[i - 1]);

    #undef DO_LINE
    #undef TRANSITION
}
Improve this answer
\$\endgroup\$
0

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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