Game of Life class

Question

I'm a beginner in C++, and I'd like you to review the class I wrote for the Game Of Life.

GameOfLife.h

#include <vector>
#include <iostream>
using namespace std;

class GameOfLife{

    public:
        GameOfLife(void);
        GameOfLife(const unsigned &, const unsigned &);
        ~GameOfLife(void);
        void display(void);
        void swapCell(const unsigned &,const unsigned &);
        void updateField();

    private:
        unsigned Xmax, Ymax;
        unsigned XSize, YSize;
        vector< vector<unsigned> > field;
        unsigned newValCell(const unsigned &, const unsigned &);
};

GameOfLife.cpp

#include "GameOfLife.h"

GameOfLife::GameOfLife(void) {
    Xmax = 10;
    Ymax = 10;
    XSize = Xmax + 2;
    YSize = Ymax + 2;
    for (unsigned i = 0; i != XSize; i++)
    {
        vector<unsigned> inter;
        for (unsigned j = 0; j != YSize; j++)
        {
            inter.push_back(0);
        }
        field.push_back(inter);
    }
}

GameOfLife::GameOfLife(const unsigned &x, const unsigned &y) {
    Xmax = x;
    Ymax = y;
    XSize = Xmax + 2;
    YSize = Ymax + 2;
    for (unsigned i = 0; i != XSize; i++)
    {
        vector<unsigned> inter;
        for (unsigned j = 0; j != YSize; j++)
        {
            inter.push_back(0);
        }
        field.push_back(inter);
    }
}

GameOfLife::~GameOfLife(void)
{
    return;
}

void GameOfLife::swapCell(const unsigned &x, const unsigned &y)
{
    field[x][y] = !field[x][y];
    return;
}

// return the new value of a cell depending on the surrounding values
unsigned GameOfLife::newValCell(const unsigned &x, const unsigned &y)
{
    unsigned stok;
    stok = field[x-1][y-1]
         + field[x-1][y]
         + field[x-1][y+1]
         + field[x][y-1]
         + field[x][y+1]
         + field[x+1][y-1]
         + field[x+1][y]
         + field[x+1][y+1];

    switch(stok)
    {
        case 3: return 1;
        case 2: return field[x][y];
        default: return 0;
    }
}

void GameOfLife::updateField(void)
{
    vector< vector<unsigned> > stockField = field;
    for (unsigned i = 1; i != Xmax + 1; i++)
    {
        for (unsigned j = 1; j != Ymax + 1; j++)
        {
            stockField[i][j] = newValCell(i,j);
        }
    }
    field = stockField;
    return;
}

void GameOfLife::display(void)
{
    for (int i = 1; i != Xmax + 1; i++)
    {
        for (int j = 1; j != Ymax + 1; j++)
        {
            cout << field[i][j] << " ";
        }
        cout << "\n";
    }
    return;
}

main()

#include "GameOfLife.h"

int main()
{
    GameOfLife a = GameOfLife(7,7);
    a.swapCell(1,5);
    a.swapCell(2,5);
    a.swapCell(3,5);
    a.swapCell(3,6);
    a.swapCell(2,7);
    a.display();
    cout << "-------\n";
    a.updateField();
    a.display();
    cout << "-------\n";
    a.updateField();
    a.display();
    cout << "-------\n";
    a.updateField();
    a.display();
    cout << "-------\n";
    a.updateField();
    a.display();
    cout << "-------\n";
    a.updateField();
    a.display();
    return 0;
}

Answer 1

You use unsigned as your data storage you should instead use bool, std::vector<bool> is specialized to use a bit vector for more efficient storage.

Initialization of the field can happen in just 2 calls:

vector<bool> inter = vector<bool>(YSize, false);
field = vector<bool>(XSize, inter);

Speaking of the field, consider flattening the field so the field has size YSize*XSize and you index into it with y*XSize+x (which you can put in a helper method).

newValCell will happily go out of bounds and not notify you about it.

The for loops in updateField and display are 1-based while they should be 0-based (leads to an overflow).

swapCell is not well named, toggleCell or invertCell would be better, a explicit setCell and unsetCell wouldn't go amiss either.

You should allow the program to query the field state using a getCell method. So its not bound to a command line program.

Answer 2

Here are some things that may help you improve your code:

Don't abuse using namespace std

Putting using namespace std at the top of every program is a bad habit that you'd do well to avoid. It is particularly bad to put it into a header file, so please don't do that.

Eliminate unused variables

The XSize and YSize variables are only used once in the constructor and can easily be eliminated.

Prefer modern initializers for constructors

The constructors can be collapsed to a single one with default values and a parameter intialization style. The declaration can have the default 10 x 10 size:

GameOfLife(const unsigned &x=10, const unsigned &=10);

The implementation can be much simplified by using std::vector constructors that take a count, value pair:

GameOfLife::GameOfLife(const unsigned &x, const unsigned &y) :
    Xmax(x), Ymax(y), field(Xmax+2, vector<bool>(Ymax+2, 0))
{
}

Consider using bool instead of int for field

On my machine, the current code allocates a total of 3276 bytes, but that is reduced to 2600 if the type of field values is changed from int to bool.

Reconsider function and field names

The name swapCell would probably be better named toggleCell because it's not really swapping with anything in the sense of std::swap. Also, the naming of Xmax and XSize with lowercase 'm' and uppercase 'S' respectively seems a little inconsistent.

Let the compiler create default destructor

The compiler will create a destructor by default which is essentially identical to what you've got, so you can simply omit both the declaraton and implementation from your code.

Omit return 0

When a C++ program reaches the end of main the compiler will automatically generate code to return 0, so there is no reason to put return 0; explicitly at the end of main. Similarly, there's no good reason to put return; at the end of a void function.

Be careful with signed and unsigned

In the current display routine, the loop integers i and j are both signed int values, but they're being compared with unsigned quantities Xmax and Ymax. Better would be to declare them all as unsigned or perhaps size_t.

Use const where practical

The current display() routine does not (and should not) modify the underlying object, and so it should be declared const:

    void display(void) const;

Less obviously, perhaps, the newValCell() helper function should also be const.

Use an ostream &operator<< instead of display

The current code has void GameOfLife::display(void) but what would make more sense and be more general purpose would be to overload an ostream operator<< instead. The body of the existing display routine would only need small modifications:

friend std::ostream &operator<<(std::ostream &out, const GameOfLife &game)
{
    for (unsigned i = 1; i != game.Xmax + 1; i++)
    {
        for (unsigned j = 1; j != game.Ymax + 1; j++)
        {
            out << game.field[i][j] << " ";
        }
        out << "\n";
    }
    return out;
}

Then the main routine could use this:

cout << a << "-------\n";

instead of this:

a.display();
cout << "-------\n";

Reconsider the interface

Each call to updateField is something of an increment, so one possibility would be to declare it as an increment operator:

GameOfLife &operator++() { updateField(); return *this; }

If you did so and used the previous suggestion, the last part of the code for main could look like this:

cout << a << "-------\n";
for (int i=0; i < 5; ++i) 
{
    cout << ++a << "-------\n";
}