4
\$\begingroup\$

Does my code for game of life look good enough? What changes can I make to optimize the code?

I am a beginner to java and I've coded the Game of Life! For those not familiar, the game entails creating a grid of specified dimensions with each box in the grid being either dead or alive. The grid starts with a random dead or alive state for each box. New generations have new dead/alive boxes based on the following conditions: If there are 3 alive boxes neighbouring any dead box, it becomes alive. If there are less than 2 alive boxes neighbouring an alive box, that box dies. It also dies if there are more than 3 alive boxes around it.

#include<cstdlib>
#include<ctime>

const int Grid_Width = 10;
const int Grid_Height = 10;
int Grid [Grid_Width][Grid_Height]; //2-D array to hold the grids


//initialize grid with random cell states
void initializegrid() {
    std::srand(std::time(nullptr)); //seed the random number generator
    for(auto i=0; i<Grid_Width;++i){
        for(auto j=0; j<Grid_Height; ++j){
            Grid[i][j]=std::rand() % 2; //gives each cell a value of 1 or 0
        }
    }
}

//function to print the grid to the console
void print_grid() {
    for(auto i=0; i<Grid_Width;++i){
        for(auto j=0; j<Grid_Height; ++j){
            std::cout<< (Grid[i][j] == 1 ? "*" : ".")<<" "; //print * for live cell and . for dead one 
        }
        std::cout<<std::endl;
    }
}

//function to count live neihghbour
int countliveNeighbour( int x, int y){
    int count =0;
    for(auto i=-1; i<=1; ++i){
        for(auto j=-1; j<=1; ++j){ //covering 3X3 grid
            int neighbourX = x + i;
            int neighbourY = y + j;
            if(i==0 && y==0) continue; //skipping the cell itself
            if(neighbourX>=0 && neighbourX< Grid_Width && neighbourY>=0 && neighbourY< Grid_Height ){
                count+= Grid[neighbourX][neighbourY];
            }
        }
    }
    return count;
}

//function to update the grid  based on the rules of the game
void updateGrid(){
    int newGrid[Grid_Width][Grid_Height];
    for(auto i=0; i<Grid_Width; ++i){
        for(auto j=0; j<Grid_Height; ++j){
            int liveNeighbours = countliveNeighbour(i,j);
            if(Grid[i][j]==1){
                if(liveNeighbours <2 || liveNeighbours >3){
                    newGrid[i][j]=0; //cell dies due to underpopulation
                }else{
                    newGrid[i][j]=1;
                }
            }else{
                if(liveNeighbours ==3){
                    newGrid[i][j]=1; //cell becomes alive due to overpopulation
                }else{
                    newGrid[i][j]=0; //remains dead
                }
            }
        }
    }
    //update the old grid with the new grid
    for(auto i=0; i<Grid_Width; ++i){
        for(auto j=0; j<Grid_Height; ++j){
            Grid[i][j]=newGrid[i][j];
        }
    }
}






int main(){

    initializegrid();
    print_grid();

    updateGrid();
    print_grid();
}
\$\endgroup\$
2
  • 6
    \$\begingroup\$ I am a beginner to java ... do you mean C++? \$\endgroup\$
    – toolic
    Mar 18 at 19:42
  • \$\begingroup\$ See this post I just made: codereview.stackexchange.com/q/291108/507 see if you can use that to help you simplify the animation (assumes you are using an X-Term based terminal). \$\endgroup\$ Mar 20 at 17:03

2 Answers 2

3
\$\begingroup\$

Everything Toby Said.
So not going to repeat those.


Added:
Game play improvement:

I would not do this. It means that that things die off at the edges of the grid.

        if(neighbourX>=0 && neighbourX< Grid_Width && neighbourY>=0 && neighbourY< Grid_Height ){
            count+= Grid[neighbourX][neighbourY];
        }

I would rather think of the grid as wrapping around on both the X and Y axis. That way life things like gliders will wrap around the edge rather than die as they hit the edge.

Try this:

        count+= Grid[neighbourX % Grid_Width][neighbourY % Grid_Height];

Can't remember the exact rules on % and negative number you should test. But if it does not work as expected you can fix like this:

        count+= Grid[(neighbourX + Grid_Width) % Grid_Width][(neighbourY + Grid_Height) % Grid_Height];

A lot of life games produces output that would go out of the viable area quickly and thus leave a boring blank screen, this will allow the simulation a better chance of continuing.



I personally dislike global state (the mutable kind anyway).
It makes debugging a lot harder when you have to worry about state that is not part of your function.

So rather than:

const int Grid_Width = 10;
const int Grid_Height = 10;
int Grid [Grid_Width][Grid_Height]; //2-D array to hold the grids

I would prefer you wrap this in a class and create an instance of the class in main().

class Life
{
    constexpr int Grid_Width = 10;
    constexpr int Grid_Height = 10;

    int Grid [Grid_Width][Grid_Height]; //2-D array to hold the grids
    public:
        Life() // Initialize here once.
        // etc

};

Rather than using a C-Array:

int Grid [Grid_Width][Grid_Height];

I would probably go with a std::vector<>

I would probably use bool rather than int for storage. I think this would work (But I am not going to look it up). But the standard optimizes space for std::vector<bool> and not all functionality works exactly the same, so if it does not work as expected change bool to int.

std::vector<std::vector<bool>>  grid;
// 
// In constructor
Life()
    : grid(Grid_Width, std::vector<int>(Grid_Height, false));
{}

This makes things like copying simpler and your updateGrid could have one loop removed with and simply assign the arrays.

      //update the old grid with the new grid
      grid = std::move(newGrid);

In the update grid I would have done it slightly differently.

I would have initialized all the members to 0. Then the code only needs to worry about updating the cells that are going to be live. I think this simplifies the code. And use a named function to make the decision.

bool isAlive(int currentState, int liveNeighbours) {

    return
        (currentState == 1 && (liveNeighbours == 2 || liveNeighbours == 3) 
    ||  (currentState == 0 && liveNeighbours == 3);
}

void updateGrid(){
    // Initialize grid to all zero.
    int newGrid[Grid_Width][Grid_Height] = {0};


    // Find live squares.
    for(auto i=0; i<Grid_Width; ++i){
        for(auto j=0; j<Grid_Height; ++j){
            int liveNeighbours = countliveNeighbour(i,j);
            if (isAlive(grid[i][j], liveNeighbours)) {
                newGrid[i][j] = 1;
            }
        }
    }

    //update the old grid with the new grid
    for(auto i=0; i<Grid_Width; ++i){
        for(auto j=0; j<Grid_Height; ++j){
            Grid[i][j]=newGrid[i][j];
        }
    }
}
\$\endgroup\$
3
\$\begingroup\$

This is pretty good for beginner code; I think you'll do well in C++.


We need constexpr int Grid_Width (and same for height) to be allowed to use them as dimensions for the Grid array. Perhaps you're invoking the compiler in a very lenient mode? Most compilers need supplying with options to constrain them to enforce standards and/or provide good warnings.


Whilst not wrong per se, the use of std::rand() in initializegrid can be replaced with the more modern facilities in <random>. Doing so will not only give more flexibility (e.g. to change the proportion of filled cells) but will also give more uniform distribution (std::rand() implementations are often weak in the low-order bits).


The use of std::endl is rarely desired. std::cout << std::endl is equivalent to std::cout << '\n' << std::flush, and in this case we don't (currently) need the flush, as the program is about to terminate, which flushes output anyway. If you're going to need it to flush in a later iteration, I recommend writing it explicitly so readers see that it is intentional.


In countliveNeighbour() (strange capitalisation?), I think we have a typo here:

if(i==0 && y==0) continue;

I think we should be testing j, not y.

We could make this function a bit simpler at the expense of a little more memory. Instead of the complex test for the edge of the grid, we could over-size the grid by 1 pixel all round. If we always set these pixels to zero, and never write to them, then we don't need the test that stops us reading outside the bounds of the grid.


Style-wise, I think most C++ coders prefer more space around keywords and operators. For example:

for (auto i = 0;  i < Grid_Width;  ++i) {
\$\endgroup\$
4
  • \$\begingroup\$ Just a note. Changing i to j in if(i==0 && y==0) continue; will lead to a condition where life is created and dies in a never-ending loop. A minimum of 9 cells that do it are * . . on one line, with * . * directly below and finally . * * below that. \$\endgroup\$ Mar 22 at 22:17
  • \$\begingroup\$ In fact, that's an endless loop of life no matter which way you go, i or j. \$\endgroup\$ Mar 22 at 23:41
  • \$\begingroup\$ I wrote y to j, not i to j. I don't know if that causes a loop or not, but it does at least then agree with the comment. \$\endgroup\$ Mar 23 at 17:12
  • \$\begingroup\$ My bad it was y and j I was talking about -- funny how the brain has i and j ingrained from years of nested loops :) \$\endgroup\$ Mar 24 at 2:31

Your Answer

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

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