Java console version Conway's Game of Life

Question

I did a super simple version of Conway's Game of Life and would like a review.

public class Conway {
        String [][] grid;
        int [][] neighbors;
        int [][] directions = {{0,1},{1,1},{1,0},{1,-1},{0,-1},{-1,-1},{-1,0},{-1,1}};
        Conway (int gridSize) {
                this.neighbors = new int [gridSize][gridSize];
                this.grid = new String[gridSize][gridSize];
                for (int row = 0; row < this.grid.length; row++) {
                        for (int cell = 0; cell < this.grid[row].length; cell++) {
                                this.grid[row][cell] = (Math.random() > .9) ? "x|" : " |";
                                System.out.print(this.grid[row][cell]);
                        }
                        System.out.println();
                }
                this.getNeihbors();
        }
        void getNeihbors() {
                int i = 0;
                for (int row = 0; row < this.grid.length; row++ ) {
                        for (int col = 0; col < this.grid[row].length; col++) {
                                for (int drow = 0; drow < this.directions.length; drow++) {
                                        int x = (this.directions[drow][this.directions[drow].length-this.directions[drow].length]);
                                        int y = (this.directions[drow][this.directions[drow].length - 1]);
                                        x = row + x;
                                        y = col + y;
                                        if (x < 0) {
                                                x += this.grid.length;
                                        } else if (x + 1 > this.grid.length) {
                                                 x -= x;
                                        }
                                        if (y < 0) { 
                                                y += this.grid[row].length;
                                        } else if (y + 1 > this.grid[row].length) {
                                                 y -= y;
                                        }
                                        if (this.grid[x][y] == "x|") i++;
                                }
                                this.neighbors[row][col] = i;
                                i=0;
                        }
                }
                this.showGrid();
        }
        void showGrid() {
                for (int row = 0; row < this.grid.length; row++) {
                        for (int cell = 0; cell < this.grid[row].length; cell++) {
                                if (this.grid[row][cell] == " |" && this.neighbors[row][cell] == 3) {
                                        this.grid[row][cell] = "x|";
                                } else if (this.grid[row][cell] == "x|" && this.neighbors[row][cell] > 3) {
                                        this.grid[row][cell] = " |";
                                } else if (this.grid[row][cell] == "x|" && this.neighbors[row][cell] < 2) {
                                        this.grid[row][cell] = " |";
                                }
                                System.out.print(this.grid[row][cell]);
                        }
                        System.out.println();
                }
                this.getNeihbors();
        }
        public static void main(String[] args) {
                Conway c = new Conway(80);
        }
}

Answer 1

Object fields

    String [][] grid;
    int [][] neighbors;

I would make both of these private rather than the default package-protected visibility. Unless of course you are actually planning to access these from another class in the package.

    private String [][] grid;
    private int [][] neighborCounts;

I also changed the name of neighbors, as I found the original inaccurate. I would expect a variable called neighbors to hold some kind of reference to the neighbors. Instead, this counts the neighbors, which is necessary for Conway's Game of Life. So let's call it what it is.

I'd also consider changing the grid to be some other kind of object rather than String. But that's something of a judgment call. One advantage of doing that would be that you could get rid of neighbors altogether. Instead, each cell would track its own neighbors.

Class constants

        int [][] directions = {{0,1},{1,1},{1,0},{1,-1},{0,-1},{-1,-1},{-1,0},{-1,1}};

You make this a regular object field, but it never changes and is the same for all instances of the class.

        private static final int [][] directions
                = {{0,1}, {1,1}, {1,0}, {1,-1}, {0,-1}, {-1,-1}, {-1,0}, {-1,1}};

Now only one copy will be initialized for the application.

Constructor initialization

                this.neighbors = new int [gridSize][gridSize];
                this.grid = new String[gridSize][gridSize];

Personally, I think that eight-column indent is a bit much.

            grid = new String[gridSize][gridSize];
            neighborCounts = new int[grid.length][grid[0].length];

Four-column indent is more common in Java.

If you do use an eight-column indent, consider adopting the same rule as the Linux source: only two levels of indent inside a function. That will help keep function contents from shooting off the edge. It also helps keep functions simple, without too much nested logic.

I prefer not to use the this. with object fields. Java doesn't require it, so I only use it when necessary to disambiguate from parameters with the same name.

You want neighborCounts to be the same size as grid. So make sure of that by setting it to the same dimensions dependently rather than in parallel. Parallel logic is fragile.

Don't run inside the constructor

At the end of the constructor, you

                this.getNeihbors();

First, that's misspelled (missing a 'g'). It should be getNeighbors.

Second, it's not idiomatic. I would expect a method called getNeighbors to return the contents of an object field neighbors which holds the neighbors of something. This fills out the neighbors multidimensional array. I would probably call it countNeighbors.

Third, if you start the simulation from the constructor, then you have no way of constructing an object without starting the simulation.

Consider removing all current calls to getNeighbors and showGrid and instead adding to main something like

        while (true) {
            c.countNeighbors();
            c.showGrid();
        }

That also has the advantage of not filling up the stack with recursive calls.

Simplifying

                                for (int drow = 0; drow < this.directions.length; drow++) {
                                        int x = (this.directions[drow][this.directions[drow].length-this.directions[drow].length]);
                                        int y = (this.directions[drow][this.directions[drow].length - 1]);
                                        x = row + x;
                                        y = col + y;

This could be as simple as

                for (int[] direction : directions) {
                    int y = row + direction[0];
                    int x = col + direction[1];

And

                                        } else if (x + 1 > this.grid.length) {
                                                 x -= x;

could be

                    } else if (x >= grid.length) {
                        x -= grid.length;

Now if you change the rules to allow neighbors that aren't adjacent, this will still work.

And we don't have to do an extra, unnecessary math operation.

Descriptive names

                int i = 0;

I would find this easier to follow as

        int count = 0;

When I see a variable named i, I expect it to be a loop iteration variable. This isn't. We're counting the neighbors. Why not name it based on that?

Answer 2

Not sure what kind of feedback you are looking for so I'll break it up into sections:

Overall computation:

Your overall algorithm is sound. Clearly from your code (example else if (x + 1 > this.grid.length)) you have coded a wrap-around such that effects that go off the top wrap around to the bottom, bottom to top, left to right, and right to left.

Code style:

I'd recommend against storing the space as "x|" and " |". In other implementations I've seen the alive space stored initially as the integer 1 that gets incremented ever time it survives from one generation to the next, whereas a dead space is stored as the integer 0. The printing of "x|" and " |" is part of the "view" and would do well to be decoupled and pulled out into a separate class.

The getNeighbours function could have the inner most loop

for (int drow = 0; drow < this.directions.length; drow++) {

pulled out into a separate function called something like updateNeighbourCountForOneCell. Having a very focused function that just checks one cell will help with unit testing.

Clarity of code:

The section of code y -= y; and x -= x;, you are really just setting y and x to 0. Declaring that specifically will be clearer. Similarly, your grid is a square of gridSize by gridSize, yet you check the variable grid for its size instead of directly storing gridSize and using it. Using gridSize makes it clear that your grid is a square.