1
\$\begingroup\$

This is another out-of-the-head-and-into-the-compiler exercise. This was straight from the editor to code review. I would like suggestions on things to keep in mind when doing such exercises.

class Cell {
boolean isAlive;
int numberOfNeighbors;
}
public class Conwayv2 {
Cell [][] grid;
int gridHeight;
int gridWidth;
static final int [][] directions = {{0,1},{1,1},{1,0},{1,-1},{0,-1},{-1,-1},{-1,0},{-1,1}};

Conwayv2(int height,int width) {
    gridHeight = height;
    gridWidth = width;
    grid = new Cell[height][width];
    for (int row = 0; row < gridHeight; row ++) {
        for (int col = 0; col < gridWidth; col++) {
            Cell cell = new Cell();
            cell.isAlive = (Math.random() > .7) ? true : false;
            grid[row][col] = cell;
        }
    }
}
int countNeighbors(int r, int c) {
    int neighborsCount = 0;
    int x = 0;
    int y = 0;
    for (int [] dir:directions) {
        x = dir[0];
        y = dir[1];
        x = r + x;
        y = c + y;
        if (x >= gridHeight) {
            x -= x;
            else if (x < 0) {
                x += gridHeight;
            }
            if (y >= gridHeight) {
                y -= y;
            } else if (y < 0) {
                y += gridWidth;
            }
            if (grid[x][y].isAlive) neighborsCount++;
        }
        return neighborsCount;
    }
    boolean updateIsAlive(int r, int c) {
        if (!grid[r][c].isAlive && grid[r][c].numberOfNeighbors == 3) {
            return true;
        } else if (grid[r][c].isAlive && grid[r][c].numberOfNeighbors > 3) {
            return false;
        } else if (grid[r][c].isAlive && grid[r][c].numberOfNeighbors < 2) {
            return false;
        }
        return grid[r][c].isAlive;
    }
    void updateGrid() {
        for (int row = 0; row < gridHeight; row++) {
            for (int col = 0; col < gridWidth; col++) {
                grid[row][col].numberOfNeighbors = countNeighbors(row, col);
                grid[row][col].isAlive = updateIsAlive(row, col);
            }
        }
    }
    void displayGrid() {
        for (int row = 0; row < gridHeight; row++) {
            for (int col = 0; col < gridWidth; col++) {
                System.out.print((grid[row][col].isAlive) ? "x|" :  " |");
            }
            System.out.println();
        }
    }
    public static void main(String[] args) {
        Conwayv2 c = new Conwayv2(10,10);
        while (true) {
            c.updateGrid();
            c.displayGrid();
        }
}

}

\$\endgroup\$
  • \$\begingroup\$ grid[row][col].numberOfNeighbors = countNeighbors(row, col); grid[row][col].isAlive = updateIsAlive(row, col); IMHO this will not work in the same loop. you already changed the state of the prevoius cell neigbours. You need to do this in separate loops. \$\endgroup\$ – Timothy Truckle Oct 27 '16 at 16:13
  • 1
    \$\begingroup\$ When I do this I usually implement the directions as enum. This brings the boundary checks to a more appropriate place (one per direction). I also have the celles knowing their neigbous (collection) and listen to their state change. This enables me to update only those cells, where neigbours actually changed state, which is a big performance boost... \$\endgroup\$ – Timothy Truckle Oct 27 '16 at 16:22
  • \$\begingroup\$ You're right. One should keep performance in mind when doing such an exercise. I was just trying to quickly get the thing to work. \$\endgroup\$ – Jim Ferguson Oct 27 '16 at 16:53
  • \$\begingroup\$ "One should keep performance in mind when doing such an exercise." But never secrifice readability for performance unless you have a proove that it is both, nessessarry and real. \$\endgroup\$ – Timothy Truckle Oct 27 '16 at 18:40

Your Answer

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

Browse other questions tagged or ask your own question.