3
\$\begingroup\$

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);
        }
}
\$\endgroup\$
  • \$\begingroup\$ You tagged this with comparative-review but posted only one solution, what do you want this to be compared to? \$\endgroup\$ – I'll add comments tomorrow Oct 24 '16 at 23:41
  • 1
    \$\begingroup\$ How many different ways are there to spell "neighbors" in this program? \$\endgroup\$ – Eric Lippert Oct 25 '16 at 18:32
  • \$\begingroup\$ Spell check really wasn't the answer I was looking for. \$\endgroup\$ – Jim Ferguson Oct 26 '16 at 3:29
  • \$\begingroup\$ Naming does matter, and in programming naming is hard. It's not "spell check", it's a rethorical way of pointing out that deliberately making code look like it's riddled with typos might not be the best of ideas in terms of maintainability and usability. \$\endgroup\$ – Mathieu Guindon Oct 26 '16 at 4:46
3
\$\begingroup\$

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?

\$\endgroup\$
  • \$\begingroup\$ Thank you very much. This is exactly what I was looking for. I did this as an exercise to see how quickly I could write a simple implementation of Conway's game without reference to anyone else's code. I considered using a cell object but settled for the neighbors grid because it was the first thing I thought of. (The exercise was to see how fast I could get it working.) \$\endgroup\$ – Jim Ferguson Oct 25 '16 at 13:21
1
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Thank you for your help. This code was an exercise in speed. Basically it came straight out of my head and into the compiler. Your suggestions will help me with the next exercise of this type. \$\endgroup\$ – Jim Ferguson Oct 25 '16 at 13:49
  • \$\begingroup\$ Curious why I got downvoted. Any feedback would be helpful so I can improve my future responses. \$\endgroup\$ – Panchishin Oct 25 '16 at 14:51
  • \$\begingroup\$ I did not downvote you. However, I would disagree with "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." The general recommendation is to use grid.length because then it will stay consistent even in the face of other changes, e.g. switching from a square to a rectangle. Your recommendation hardens the requirement that it is a square. It's usually better to be more flexible rather than less. \$\endgroup\$ – mdfst13 Oct 25 '16 at 15:27

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.