5
\$\begingroup\$

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;
}
\$\endgroup\$
  • \$\begingroup\$ Don't put void in the ( ) parameter list if the function takes 0 arguments, that's just useless boilerplate. Also, keeping the parameter names in function prototypes helps documenting the code. \$\endgroup\$ – glampert Jan 21 '15 at 16:54
  • \$\begingroup\$ GameOfLife::GameOfLife(void) { is really the same as GameOfLife(10,10);. Avoid copy-pasted code. \$\endgroup\$ – njzk2 Jan 21 '15 at 18:10
6
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ the for loops goes from 1 to X/Ymax because the borders of the field are set to a constant value 0 and are never updated neither displayed. (or i misinterpret your comment?). \$\endgroup\$ – youyou Jan 21 '15 at 12:59
  • \$\begingroup\$ @user59330 then why +1 instead of -1 when adjusting the loop condition? \$\endgroup\$ – ratchet freak Jan 21 '15 at 13:07
  • \$\begingroup\$ not sure but i think you confound Xmax (calculated field length) and XSize (array length). \$\endgroup\$ – youyou Jan 21 '15 at 13:11
  • \$\begingroup\$ @user59330 add that to the list, keeping the bounds twice, you only need it once \$\endgroup\$ – ratchet freak Jan 21 '15 at 13:14
  • \$\begingroup\$ thx for your comments :) \$\endgroup\$ – youyou Jan 21 '15 at 13:16
7
\$\begingroup\$

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";
}
\$\endgroup\$
  • \$\begingroup\$ thx for your comments! how do you monitor the memory use? \$\endgroup\$ – youyou Jan 21 '15 at 15:41
  • 1
    \$\begingroup\$ I use valgrind for that. Very handy tool! \$\endgroup\$ – Edward Jan 21 '15 at 15:44
  • 1
    \$\begingroup\$ Input parameters that are basic types (int, float, bool etc) should be passed by value. I didn't know the standard mandated a default return 0 in main(), but I would still like to see explicit returns :) \$\endgroup\$ – Emily L. Jan 21 '15 at 16:29
  • \$\begingroup\$ Omit return 0 - I read it more then once that it is actually a good style to always do that in main (unless you want to return something else ofc). \$\endgroup\$ – Cthulhu Jan 21 '15 at 17:07
  • \$\begingroup\$ @EmilyL. good point about passing by value -- I neglected to mention that. \$\endgroup\$ – Edward Jan 21 '15 at 17:33

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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