8
\$\begingroup\$

Yet another Game Of Life implementation code review question

I am attempting to learn SDL, and have a general (beginner) knowledge of c++. I am not a developer by trade so I expect my implementation and approach is naive.
Some specific questions I have about my code are as follow

  1. In Game.cpp, GameStateRunning will leak memory when the fps surface or texture is declared outside of the function scope. Instantiating and destroying them with each frame is costly, how can this be avoided?
  2. Is a 2D array the correct data structure to utilize?
  3. How can the number of iterations through the grid be reduced?
  4. How can I improve my attempts at OOP and DRY?

Game.h

#pragma once
#include <SDL.h>
#include <SDL_ttf.h>
#include <stdlib.h>
#include <iostream>
#include <string>
#include <time.h>

#include "Cell.h"
#include "Menu.h"

typedef enum 
{
    RUNNING,
    PAUSED,
    MENU
} game_state_types;


class GameOfLife
{
    public:
        //Constructors & Destructors
        GameOfLife();
        ~GameOfLife();

        //Do Things
        int Play();
        int RandomizeBoard(float percentAlive);
        int TimeStep(void);
        int RenderGrid(void);
        int GridGetNeighbors(int x, int y);
        cell_states GridApplyRules(int currState, int numNeighbors);

        //Get and set
        int GetGridWidth(void);
        int SetGridWidth(int width);
        int GetGridHeight(void);
        int SetGridHeight(int height);
        int GetIsRunning(bool *running);
        std::string GetGameError(void);


    private:
        int gridWidth;
        int gridHeight;
        Cell **theGrid;
        int DeleteGrid();
        int CreateGrid();
        SDL_Rect cellToRender;

        bool isRunning;
        game_state_types gameState;

        int GameStateRunning();
        int GameStatePaused();
        int GameStateMenu();

        Menu *gameMenu;

        SDL_Event gameEvent;
        SDL_Window *gameWindow;
        SDL_Renderer *gameRenderer;
        SDL_Surface *gameWindowSurface;
        int gameWindowWidth, gameWindowHeight;
        int gameWindowXPos, gameWindowYPos;

        TTF_Font *Sans;


        std::string gameError;

        //For FPS
        const float FPS_INTERVAL = 1.00;
        const float UPDATE_INTERVAL = 0.25;
        uint32_t updatePrevTick;
        uint32_t fpsPrevTick;
        uint32_t fpsCurrent;
        uint32_t fpsFrames; //numbers of frames that have occured
};

Game.cpp

#include "Game.h"


GameOfLife::GameOfLife()
{
    isRunning = true;
    gameState = RUNNING;

    gameWindow = SDL_CreateWindow("Game Of Life", SDL_WINDOWPOS_CENTERED, SDL_WINDOWPOS_CENTERED, 1280, 720, SDL_WINDOW_SHOWN | SDL_WINDOW_OPENGL | SDL_WINDOW_RESIZABLE);
    if (gameWindow == NULL)
    {
        gameError = "SDL Failed to create window!";
        exit(-1);
    }

    gameRenderer = SDL_CreateRenderer(gameWindow, -1, SDL_RENDERER_ACCELERATED);
    if (gameRenderer == NULL)
    {
        gameError = "SDL Failed to create renderer!";
        exit(-1);
    }

    SDL_SetRenderDrawBlendMode(gameRenderer, SDL_BLENDMODE_BLEND);

    Sans = TTF_OpenFont("OpenSans-Regular.ttf", 24);

    gameWindowSurface = SDL_GetWindowSurface(gameWindow);

    SDL_GetWindowSize(gameWindow, &gameWindowWidth, &gameWindowHeight);

    gridWidth = 128;
    gridHeight = 72;

    gameMenu = new Menu(gameWindowWidth, gameWindowHeight, gridWidth, gridHeight, Sans);

    CreateGrid();

    updatePrevTick = 0;
    fpsPrevTick = 0;
    fpsCurrent = 0;
    fpsFrames = 0; 
}

GameOfLife::~GameOfLife()
{
    isRunning = false;

    gameWindow = NULL;
    gameRenderer = NULL;
    gameWindowSurface = NULL;

    SDL_DestroyWindow(gameWindow);
    SDL_DestroyRenderer(gameRenderer);
    SDL_FreeSurface(gameWindowSurface);

    DeleteGrid();

    TTF_CloseFont(Sans);
}

int GameOfLife::Play()
{
    SDL_SetRenderDrawColor(gameRenderer, 255, 255, 255, 255);
    SDL_RenderClear(gameRenderer);

    //Check user inputs
    if (SDL_PollEvent(&gameEvent) != 0)
    {
        switch (gameEvent.type)
        {
        case SDL_QUIT:
            isRunning = false;
            break;
        case SDL_WINDOWEVENT:
            if (gameEvent.window.event == SDL_WINDOWEVENT_SIZE_CHANGED)
            {
                gameWindowWidth = gameEvent.window.data1;
                gameWindowHeight = gameEvent.window.data2;
                gameMenu->WindowResized(gameWindowWidth, gameWindowHeight);
            }
            break;
        case SDL_MOUSEBUTTONDOWN:
            int mouseX, mouseY;
            SDL_GetMouseState(&mouseX, &mouseY);
            if (gameMenu->CheckMenuPress(mouseX, mouseY) == OPEN_MENU_BTN)
                gameState == RUNNING ? gameState = MENU : gameState == PAUSED ? gameState = MENU : gameState = RUNNING;
            break;
        case SDL_KEYDOWN:
            switch (gameEvent.key.keysym.sym)
            {
            case SDLK_ESCAPE:
                isRunning = false;
                break;
            case SDLK_r:
                RandomizeBoard(75.0);
                break;
            case SDLK_p:
                gameState == RUNNING ? gameState = PAUSED : gameState = RUNNING;
                break;
            case SDLK_m:
                gameState = MENU;
                break;
            }
            break;
        default:
            //Do nothing
            break;
        }
    }

    switch (gameState)
    {
    case RUNNING:
        GameStateRunning();
        break;

    case PAUSED:
        GameStatePaused();
        break;

    case MENU:
        GameStateMenu();
        break;

    default:
        gameError = "Unkonown game state!";
        SDL_Delay(1000);
        break;
    }

    return 1;
}

int GameOfLife::RandomizeBoard(float percentAlive)
{
    srand(time(NULL));
    for(int i = 0; i < gridWidth; i++)
        for (int j = 0; j < gridHeight; j++)
        {
            theGrid[i][j].SetNextState((cell_states)((rand() / (float)100) < percentAlive));
            theGrid[i][j].TimeStep();
        }

    return 1;
}

int GameOfLife::GameStateRunning()
{
    SDL_Texture *fpsTexture = NULL;
    SDL_Surface *fpsSurface = NULL; //Why does memory leak when I do this as a part of the class, and free it in dtor?
    SDL_Rect fpsRect = { gameWindowWidth - 75, 0, 75, 60 };

    SDL_Color textColor = { 0, 0, 0 };
    std::string fpsString = "FPS: " + std::to_string(fpsCurrent);
    fpsSurface = TTF_RenderText_Solid(Sans, fpsString.c_str(), textColor);
    fpsTexture = SDL_CreateTextureFromSurface(gameRenderer, fpsSurface);


    for (int x = 0; x < gridWidth; x++)
        for (int y = 0; y < gridHeight; y++)
        {
            int numNeighbors = GridGetNeighbors(x, y);
            theGrid[x][y].SetNextState(GridApplyRules(theGrid[x][y].GetCurrentState(), numNeighbors));
        }

    TimeStep();
    RenderGrid();
    SDL_RenderCopy(gameRenderer, fpsTexture, NULL, &fpsRect);

    SDL_SetRenderDrawColor(gameRenderer, 0x00, 0x00, 0xFF, 0x3F);
    SDL_Rect *menuButtons = gameMenu->GetButtons();
    SDL_RenderFillRect(gameRenderer, &menuButtons[0]);

    SDL_DestroyTexture(fpsTexture);
    SDL_FreeSurface(fpsSurface); //same mem leak question

    SDL_RenderPresent(gameRenderer);
    updatePrevTick = SDL_GetTicks();


    fpsFrames++;
    if (fpsPrevTick <= SDL_GetTicks() - FPS_INTERVAL * (float)1000)
    {
        fpsPrevTick = SDL_GetTicks();
        fpsCurrent = fpsFrames;
        fpsFrames = 0;
    }

    return 1;
}

int GameOfLife::GameStatePaused()
{
    if (gameEvent.type == SDL_KEYDOWN)
    {
        if (gameEvent.key.keysym.sym == SDLK_SPACE)
        {
            for (int x = 0; x < gridWidth; x++)
                for (int y = 0; y < gridHeight; y++)
                {
                    int numNeighbors = GridGetNeighbors(x, y);
                    theGrid[x][y].SetNextState(GridApplyRules(theGrid[x][y].GetCurrentState(), numNeighbors));
                }

            TimeStep();
            RenderGrid();
            SDL_RenderPresent(gameRenderer);
        }
    }
    return 1;
}

int GameOfLife::GameStateMenu()
{
    static int textBoxFocus = -1;

    gameMenu->RenderMenu(gameRenderer);

    SDL_RenderPresent(gameRenderer);

    if (gameEvent.type == SDL_MOUSEBUTTONDOWN)
    {
        int mouseX, mouseY;
        SDL_GetMouseState(&mouseX, &mouseY);
        switch (gameMenu->CheckMenuPress(mouseX, mouseY))
        {
        case -1:
            textBoxFocus = -1;
            break;
        case OPEN_MENU_BTN:
            gameState = RUNNING;
            break;
        case SET_GRID_SIZE_BTN:
            SetGridWidth(std::stoi(gameMenu->GetTextBoxValue(GRID_SIZE_X_TXT)));
            SetGridHeight(std::stoi(gameMenu->GetTextBoxValue(GRID_SIZE_Y_TXT)));
            gameMenu->GridResized(gridWidth, gridHeight);
            break;
        case SET_SCREEN_SIZE_BTN:
            SDL_SetWindowSize(gameWindow, std::stoi(gameMenu->GetTextBoxValue(SCREEN_SIZE_X_TXT)), std::stoi(gameMenu->GetTextBoxValue(SCREEN_SIZE_Y_TXT)));
            gameMenu->WindowResized(gameWindowWidth, gameWindowHeight);
            break;
        case RANDOMIZE_BTN:
            RandomizeBoard(std::stoi(gameMenu->GetTextBoxValue(RANDOMIZE_PERCENT_TXT)));
            break;
        case GRID_SIZE_X_TXT + NUM_BUTTON_TYPE:
            textBoxFocus = GRID_SIZE_X_TXT;
            break;
        case GRID_SIZE_Y_TXT + NUM_BUTTON_TYPE:
            textBoxFocus = GRID_SIZE_Y_TXT;
            break;
        case SCREEN_SIZE_X_TXT + NUM_BUTTON_TYPE:
            textBoxFocus = SCREEN_SIZE_X_TXT;
            break;
        case SCREEN_SIZE_Y_TXT + NUM_BUTTON_TYPE:
            textBoxFocus = SCREEN_SIZE_Y_TXT;
            break;
        case RANDOMIZE_PERCENT_TXT + NUM_BUTTON_TYPE:
            textBoxFocus = RANDOMIZE_PERCENT_TXT;
            break;
        default:
            break;
        }
    }

    gameMenu->HandleTextInput(gameEvent, textBoxFocus);

    return 1;
}

int GameOfLife::TimeStep()
{
    for (int i = 0; i < gridWidth; i++)
        for (int j = 0; j < gridHeight; j++)
        {
            theGrid[i][j].TimeStep();
        }
    return 1;
}


int GameOfLife::RenderGrid()
{

    float cellWidth = gameWindowWidth / (float)gridWidth;
    float cellHeight = gameWindowHeight / (float)gridHeight;

    cellWidth = round(cellWidth);
    cellHeight = round(cellHeight);

    cellToRender.w = cellWidth;
    cellToRender.h = cellHeight;

    for (int x = 0; x < gridWidth; x++)
    {
        for (int y = 0; y < gridHeight; y++)
        {
            cellToRender.x = x * cellWidth;
            cellToRender.y = y * cellHeight;

            if(theGrid[x][y].GetCurrentState() == CELL_LIVING)
                SDL_SetRenderDrawColor(gameRenderer, 0x00, 0xFF, 0x00, 0xFF);
            else
                SDL_SetRenderDrawColor(gameRenderer, 0xFF, 0xFF, 0xFF, 0xFF);

            SDL_RenderFillRect(gameRenderer, &cellToRender);
        }
    }
    return 1;
}

cell_states GameOfLife::GridApplyRules(int currState, int numNeighbors)
{
    if (currState == CELL_LIVING)
        if (numNeighbors == 2 || numNeighbors == 3)
            return CELL_LIVING;
        else
            return CELL_DEAD;

    if (currState == CELL_DEAD)
        if (numNeighbors == 3)
            return CELL_LIVING;
        else
            return CELL_DEAD;

    return CELL_DEAD;
}

int GameOfLife::GridGetNeighbors(int x, int y)
{
    int numNeighbors = 0;
    int neighborX = x, neighborY = y;

    for (int i = - 1; i <= 1; i++)
    {
        for (int j = - 1; j <= 1; j++)
        {
            if (i == 0 && j == 0)
                continue;

            neighborX = x + i;
            neighborY = y + j;

            if (x + i < 0 || x + i >= gridWidth)
                i > 0 ? neighborX = 0 : neighborX = gridWidth - 1;
            if (y + j < 0 || y + j >= gridHeight)
                j > 0 ? neighborY = 0 : neighborY = gridHeight - 1;

            numNeighbors += theGrid[neighborX][neighborY].GetCurrentState();
        }
    }
    return numNeighbors;
}

int GameOfLife::GetGridWidth()
{
    return gridWidth;
}

int GameOfLife::SetGridWidth(int width)
{
    DeleteGrid();
    gridWidth = width;
    CreateGrid();
    return 1;
}

int GameOfLife::GetGridHeight()
{
    return gridHeight;
}

int GameOfLife::SetGridHeight(int height)
{
    DeleteGrid();
    gridHeight = height;
    CreateGrid();
    return 1;
}


int GameOfLife::GetIsRunning(bool *running)
{
    *running = isRunning;
    return 1;
}

std::string GameOfLife::GetGameError()
{
    return gameError;
}

int GameOfLife::DeleteGrid()
{
    for (int i = 0; i < gridWidth; i++)
        delete[] theGrid[i];
    delete[] theGrid;

    return 1;
}

int GameOfLife::CreateGrid()
{
    theGrid = new Cell*[gridWidth];
    for (int i = 0; i < gridWidth; i++)
    {
        theGrid[i] = new Cell[gridHeight];
    }
    RandomizeBoard(50);
    return 1;
} 

Cell.h

#pragma once

typedef enum
{
    CELL_DEAD,
    CELL_LIVING,
    NUM_CELL_TYPES
} cell_states;

class Cell
{
    public:
        Cell();
        ~Cell();

        int TimeStep(void);
        int GetCurrentState(void);
        int SetNextState(cell_states);

    private:
        cell_states currentState;
        cell_states nextState;
};  

Cell.cpp

#include "Cell.h"

Cell::Cell()
{
    currentState = CELL_DEAD;
}

Cell::~Cell()
{

}

int Cell::TimeStep()
{
    currentState = nextState;
    return 1;
}

int Cell::GetCurrentState()
{
    return currentState;
}

int Cell::SetNextState(cell_states newState)
{
    nextState = newState;
    return 1;
}  

Main.cpp

#include <SDL.h>
#include <SDL_ttf.h>
#include <stdlib.h>
#include <iostream>
#include "Game.h"


int main(int argc, char *argv[])
{
    if (SDL_Init(SDL_INIT_EVERYTHING) < 0)
    {
        std::cerr << "SDL could not initialize!" << SDL_GetError() << "\n";
        return -1;
    }

    if (TTF_Init() < 0)
    {
        std::cerr << "TTF could not initialize!" << TTF_GetError() << "\n";
        return -1;
    }

    //Ensure that we close SDL and SDL TTF if the game crashes/is aborted
    atexit(SDL_Quit);
    atexit(TTF_Quit);

    GameOfLife myGameOfLife = GameOfLife();
    myGameOfLife.RandomizeBoard(80.0);

    bool gameRunning = true;

    while (gameRunning == true)
    {
        if (myGameOfLife.Play() > 0)
        {
            myGameOfLife.GetIsRunning(&gameRunning); //Check if we are still running the game
        }
        else
            gameRunning = false;
    }


    return 1;
}  

Github link for easier download/viewing

\$\endgroup\$

1 Answer 1

8
\$\begingroup\$

Turn your warnings on! I would always compile with -Wall -Wextra -pedantic -pedantic-errors :) Let's go through them:

Game.cpp:153:37: warning: missing field 'a' initializer [-Wmissing-field-initializers]
    SDL_Color textColor = { 0, 0, 0 };
                                    ^

There is an alpha component in SDL_Color, which you forgot. To avoid this, use simple braces (it's also shorter to type):

SDL_Color textColor{};

The next one is weird:

Game.cpp:316:9: warning: add explicit braces to avoid dangling else [-Wdangling-else]
        else
        ^
Game.cpp:322:9: warning: add explicit braces to avoid dangling else [-Wdangling-else]
        else
        ^

What can the compiler mean by this? Let's look at the first one:

if (currState == CELL_LIVING)
    if (numNeighbors == 2 || numNeighbors == 3)
        return CELL_LIVING;
    else // <--- problem
        return CELL_DEAD;

The compiler is suggesting that you add braces, because you might have meant:

if (currState == CELL_LIVING)
{
    if (numNeighbors == 2 || numNeighbors == 3)
        return CELL_LIVING;
}
else
    return CELL_DEAD;

Yes, it could have looked at the indentation, but it didn't. Anyways, you should add braces to clearly communicate intent.

./Game.h:66:13: warning: private field 'gameWindowXPos' is not used [-Wunused-private-field]
        int gameWindowXPos, gameWindowYPos;
            ^
./Game.h:66:29: warning: private field 'gameWindowYPos' is not used [-Wunused-private-field]
        int gameWindowXPos, gameWindowYPos;
                            ^
./Game.h:75:21: warning: private field 'UPDATE_INTERVAL' is not used [-Wunused-private-field]
        const float UPDATE_INTERVAL = 0.25;
                    ^

Please remove unused variables, there is no need to let them stay :)

The other warnings are variations of the above, so I'm not going to go into them.


Even though you asked for specific things to review (which I'll go into), here are some general comments to improve your use of C++.

  1. Headers like #include <stdlib.h> are deprecated, you should use the C ++ version of them: #include <cstdlib>.

  2. "\n" is a string. I'd say it is better to use '\n' if you only want one character (also saves a call to std::strlen).

  3. It's nice that you use std::atexit for cleanup. In the future, I expect that the standard library will have a scope guard, which will do the same thing, but better (the function will not just be called at the end of main).

  4. GameOfLife myGameOfLife = GameOfLife(); Everything after the equal character is unnecessary. GameOfLife myGameOfLife; does the same exact thing and saves characters.

    Also, if you are not in C++17, but still in C++11 or later, the former might call the move constructor (in practice, it is optimized away). In C++17, there is guaranteed copy elision so both of them are equivalent in every which way.

  5. Comparing booleans to booleans is very unnecessary while (gameRunning == true) is equivalent to while (gameRunning) and nicer IMO.

  6. typedefing structures and enums is a C thing. You don't need to do that in C++.

  7. Mark classes as final if you do not intend them to be inherited from.

  8. Mark functions as noexcept if they do not throw, or compile without exceptions.

  9. You don't need to put void to say that a function doesn't take any parameters.

  10. If the destructor does nothing, just omit it.

  11. Use the member initialization list instead of assigning member variables in the constructor body:

    Cell::Cell() : currentState(CELL_DEAD) {}
    
  12. You should initialize every member variable. In Cell, nextState has an indeterminate value. Initialize it to something like UNKNOWN or something to avoid undefined behavior.

  13. As a general rule, uppercase characters are for macros, not for enum values.

  14. Use enum classes for enums that are not intended to be flags. That gives them a separate scope and avoids name conflicts if you don't use uppercase as mentioned before.

  15. Even though #pragma once is supported by every major compiler vendor, it is a non-standard compiler extension. Consider using actual #ifndef type include guards instead.

  16. Non static const member variables make your class non-copyable (and non-moveable), which is not so good. Make them static instead.

  17. You are leaking gameWindow, gameRenderer and gameWindowSurface. You are setting them to NULL, and then call the destroy functions, but they have no effect, because you already lost the actual object instances.

    You don't need to set them to NULL, they'll get deleted anyways. Same for isRunning.

  18. Use nullptr instead of NULL.

  19. As of right now, you're calling the free functions for gameWindow, gameRenderer and gameWindowSurface in the destructor. This is ok, but what if I try to copy the class? Then, the destructor of both classes will try to destroy the same object (because the pointers point to the same object). There is something called the Rule of 5 (before it was three, but now, it's five) to "protect" against this.

    You can always do that, but a better solution would be to use std::unique_ptr, which is a smart pointer. Granted, you would still be unable to copy GameOfLife, but you'll be able to move it.

    For that, gameWindow would be defined as:

    std::unique_ptr<SDL_Window, void(*)(SDL_Window*)> gameWindow;
    

    The second template parameter is for the custom deleter (it's not delete, it's SDL_DestroyWindow).

    And when you initialize it in the member initialization list (read point somewhere about that), you need to pass the custom deleter as well as the pointer to the object:

    gameWindow(SDL_CreateWindow(/*...*/), &SDL_DestroyWindow)
    //                                    ^^^^^^^^^^^^^^^^^^
    
  20. Sorry, but this line is horrible:

    gameState == RUNNING ? gameState = MENU : gameState == PAUSED ? gameState = MENU : gameState = RUNNING;
    

    Why not just use simple if statements instead of abusing ?:?

    if (gameState == RUNNING || gameState == PAUSED)
        gameState = MENU;
    else
        gameState = RUNNING;
    

    Or use the ternary operator correctly (I would use this personally):

    gameState = gameState == MENU ? RUNNING : MENU;
    

    The same goes for this line: gameState == RUNNING ? gameState = PAUSED : gameState = RUNNING;, which would just be:

    if (gameState == RUNNING)
        gameState = PAUSED;
    
  21. Typos! "Unkonown game state!".

  22. Don't use rand() with a modulor, it isn't a uniform random number generator (also you're supposed to call srand once). Use std::uniform_real_distribution instead for floating point numbers.

  23. All of your functions seem to be returning 1 for no apparent reason. If they're not supposed to return something, don't force them to. Just make them return void.

  24. Instead of having GetIsRunning update a variable through a pointer, why not rename it IsRunning and make it return if the game is running or not.

    That way, you can simplify your main loop (and you don't need gameRunning anymore):

    while (myGameOfLife.IsRunning())
        myGameOfLife.Play();
    
  25. Mark your functions const if they are not modifying any member variable. That makes them callable on const instances of your class.

  26. Either use comments to say things the code can't communicate, or leave them out. Comments such as //Do Things, //Constructors & Destructors and //Get and Set are obvious if you look at the code, so you should remove them.

    The last comment in "Game.h" is a wonderful comment for example. The rest in that file are unnecessary. FYI if you'd gave fpsFrames a name such as totalFpsFrames, you'd also not need that comment :).

  27. To address the question comment in "Menu.cpp": That's because you are creating the objects in the loop, so you also need to delete them there. If you don't, in the next loop iteration, they'll get overridden, and you'd get a memory leak.

    Basically, you're doing this (simplified):

    int* ptr = new int();
    ptr = new int();
    delete ptr;
    

    At the second line, you'll lose the original int instance, because you lost the address by assigning ptr to another object. That is what you do in the loop. If you don't delete the objects, you will override the pointer and you'll lose the first (second, ...) object's addresses. If you store the addresses in an array, and loop over the array outside of the loop to delete each instance, then it is fine.

  28. Don't comment out code. But if you do, say why you didn't delete it.

  29. Use std::array for C style arrays, and std::vector for arrays that have a variable size. That should simplify much of your code, and be safer at the same time.

  30. In GameOfLife::GameStateRunning and in GameOfLife::GameStatePaused, you are duplicating a double for loop. Please put that in a separate function.

  31. Use pre-increment instead of post-increment. Even if the performance cost is negligible (or even not there), it better communicates intent that you don't need the old value IMO.

Now to address your questions:

In Game.cpp, GameStateRunning will leak memory when the fps surface or texture is declared outside of the function scope. Instantiating and destroying them with each frame is costly, how can this be avoided?

I suspect the cause for the memory leak is the same with gameWindow (and the others) that I mentioned earlier. If you set them to NULL, you will leak the object if you don't delete them. Delete them first, set them to NULL afterwards (if it's in a destructor, if you don't even need the latter).

I don't really know how you can improve performance, because I'm not that experience with SDL2. But there is a SO question on it, so you might want look at the answers there.

Is a 2D array the correct data structure to utilize?

I'd say yes, because you are representing a grid, which is basically a 2D array. But as mentioned, your life would be easier if you'd use a double vector instead of new[] and delete[]: std::vector<std::vector<Cell>> theGrid;

How can the number of iterations through the grid be reduced?

I can see in GameOfLife five double for loops for the grid. Let's see what we can do.

If you did what I mentioned earlier, putting the duplicate double for in a separate function, then maybe you'd notice that you always call that function and TimeStep immediately afterwards. Merging them should give you a small performance boost.

And now you notice that after the merge function, you always call RenderGrid. Merge them again! That should give you another performance boost.

Now you should only have one function that does every update and render of the cells in one go, which means only one loop instead of five! Isn't that great? :)

How can I improve my attempts at OOP and DRY?

Your objects look ok to me. For DRY, you could do better (as mentioned extensively before), but generally, you did a good job there.

\$\endgroup\$
5
  • \$\begingroup\$ Thanks for the review, Can you elaborate on points 6, 14, 19, and 27? "If you set them to NULL, you will leak the object if you don't delete them. Delete them first, set them to NULL afterwards (if it's in a destructor, if you don't even need the latter). " I am not sure if I understand this. Why can I not create a surface outside of the render loop, utilize it in the render loop, and destroy it in my destructor? \$\endgroup\$
    – NickH
    Oct 14, 2017 at 19:56
  • \$\begingroup\$ @NickH For 6, see this. For 14, see this. I'll do the others, wait a sec. \$\endgroup\$
    – Rakete1111
    Oct 14, 2017 at 20:04
  • \$\begingroup\$ @NickH You can, the problem is how you delete them. Your code in the destructor of GameOfLife is basically ptr = NULL; delete ptr; It doesn't make sense, because ptr is already NULL. There is nothing to delete. If you change them, you shouldn't be getting any leaks: delete ptr; ptr = NULL;. \$\endgroup\$
    – Rakete1111
    Oct 14, 2017 at 20:06
  • \$\begingroup\$ Oh I see, hopefully this also cuts down on the cost per frame \$\endgroup\$
    – NickH
    Oct 14, 2017 at 20:08
  • \$\begingroup\$ @NickH I changed 19 and 27, see if they make sense now :) \$\endgroup\$
    – Rakete1111
    Oct 14, 2017 at 20:15

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.