Implementing Conway's Game of Life

Question

Here is an implementation of Conway's Game of Life. My code is working fine but I want you to review and give me your suggestions of how to improve it.

These are the steps of my code:

1. Initialize the grid cells with zeros (all cells are dead at the start)
2. Then randomly change some of the grid cells to one (bring some cells to life)
3. Draw the grid
4. Update the grid, by checking which cells are going to live or die
5. Go to step 3

gameOfLife.h

#ifndef GAME_OF_LIFE_H
#define GAME_OF_LIFE_H

#include <vector>
#include <random>

class gameOfLife
{
public:
    typedef std::vector< std::vector<int>> gridVector;

private:
    int gridSize;
    gridVector mainGrid;
    gridVector *gridCopy;

public:
    gameOfLife();

public:     
    void run();
    void initGrid();
    void drawGrid();
    void updateGrid();
    void copyGrid ();
    int checkNeighbors(gridVector *grid, int x, int y);
    void nextGeneration(int x, int y, int lod);
    int randGenerate(int start, int end);

private:     
    std::random_device randDevice;
    std::mt19937 gen;
};



#endif

implementation.cpp

#include <iostream>
#include "gameOfLife.h";

gameOfLife::gameOfLife() : gridSize(15), gen(randDevice())
{
}

void gameOfLife::initGrid()
{
    for(int i = 0; i < gridSize; i++)
    {
        for(int j = 0; j < gridSize; j++)
        {
            mainGrid[i].push_back(0);
        }
    }

    for(int i = 1; i < gridSize ; i++)
    {
        for(int j = 1; j < gridSize ; j++)
        {
            int randSize     = randGenerate(2 , gridSize);
            int state       = randGenerate(0 , 2);
            int indexRow    = randGenerate(1, randSize );
            int indexColumn = randGenerate(1, randSize );

            mainGrid[indexColumn][indexRow] = state;
        }
    }
}

int gameOfLife::randGenerate(int start, int end)
{
        std::uniform_int_distribution<int> genenNum(start, end - 1);

        return genenNum(gen);
}

void gameOfLife::drawGrid()
{
    char ch = 254;
    for(int i = 1; i < gridSize - 1; i++)
    {
        for(int j = 1; j < gridSize - 1; j++)
        {

                if(mainGrid[i][j])
                {
                    std::cout << " "<<ch;
                }
                else
                {
                    std::cout << "  ";
                }

        }
        std::cout<<std::endl;
    }
}

void gameOfLife::copyGrid ()
{
    gridCopy = &mainGrid;
    (*gridCopy).resize(gridSize);
}

void gameOfLife::updateGrid()
{
    copyGrid();

    for(int i = 1; i < gridSize - 1 ; i++)
    {
        for(int j = 1; j < gridSize - 1; j++)
        {
            int lod = checkNeighbors(gridCopy, i, j);    
            nextGeneration( i, j, lod);

        }

    }
}

int gameOfLife::checkNeighbors(gridVector *gridCopy, int x, int y)
{
    int lod = 0;
     for(int i = -1; i < 2; i++)
     {
         for(int j = -1; j < 2; j++)
         {
             if(!(i == 0 && j == 0))
             {
                 if((*gridCopy)[ x + i ][ y + j ]) 
                 {
                     lod++;
                 }
             }
         }
     }

     return lod;
}

void gameOfLife::nextGeneration(int x, int y, int lod)
{
    if(lod < 2) 
    {
        mainGrid[x][y] = 0;
    }
    else if(lod == 3)
    {
        mainGrid[x][y] = 1;
    }
    else if(lod > 3)
    {
        mainGrid[x][y] = 0;
    }
}

void gameOfLife::run()
{
    mainGrid.resize(gridSize);

    initGrid();

    while(true)
    {       
        drawGrid();
        updateGrid();       
        system("cls");
    }
}

int main()
{
    gameOfLife gol;
    gol.run();

    std::cin.get();

    return 0;
}

Answer 1

• #includes should not end with a semicolon:

  #include "gameOfLife.h";
  

  The reason this is incorrect is because this is a preprocessor directive, which do not follow the same syntax as compiled lines.

• Instead of using a typedef:

  typedef std::vector< std::vector<int>> gridVector;
  

  you can now use C++11's type alias:

  using std::vector<std::vector<int>> = gridVector;
  

• If you don't need the default constructor, you can either leave it out or make it explicitly defaulted:

  gameOfLife() = default;
  

• There's really no need for multiple public and private sections. If you prefer to organize or separate the members in some way, you can just add line breaks.

• I'm not sure why you have a *gridCopy member. Not only would it be more natural to use a copy constructor in such cases, but having a raw pointer as a member would "force" you to have an overloaded copy constructor. This is because the default copy constructor performs shallow copies, which will copy the pointer instead of the value at that pointer.

  After removing this member, you should also remove copyGrid().

• I'm not too familiar with GoL, but I don't think that while loop in run() is supposed to run infinitely. This type of while loop will not stop unless a break or return is encountered. You may need to modify it in such a way that it can end.

Answer 2

Correctness: gridCopy is a pointer to mainGrid. Both names alias same data. No data is copied by gridCopy = &mainGrid assignment. Every change made by nextGeneration (allegedly on mainGrid) is visible through gridCopy. Which means the code doesn't implement Game of Life in Conway's sense. You need to create a real copy.

I'd also question a gridCopy being a class member. It is passed down as an argument to checkNeighbors, which in turn is only called from updateGrid. gridCopy is actually calculated by the latter and is not needed anywhere else. It is better be a updateGrid local.

PS: system(cls) resulted in (an expected) runtime error on my linux box.

Answer 3

Thank you @Jamal and @vnp for your help. After taking your suggestions into consideration I have made some changes to my code. First I have updated my copyGrid() method so that it can now make a real copy.

void gameOfLife::copyGrid()
{
    (*gridCopy).clear();
    (*gridCopy).resize(gridSize);

    for(int i = 0; i < gridSize; i++)
    {
        for(int j = 0; j < gridSize; j++)
        {
            (*gridCopy)[i].push_back((*mainGrid)[i][j]);
        }
    }
}

Secondly, I add a new method called swap() which swaps the gridCopy and mainGrid vectors at the end of every iteration.

void gameOfLife::swap()
{
    gridVector *temp = mainGrid;
    mainGrid = gridCopy;
    gridCopy = temp;
}

And finally, instead of running the program in an infinite loop, now the user can decide on the number of generations to do.

int generation = 500;

for (int i = 0; i < generation; i++)
{   
    drawGrid();
    updateGrid();
    system("cls");
}

And as you guys said system("cls") is not a good way to clear the screen, I am now in the middle of implementing my own and will post it when it is complete.