Skip to main content
Code Review

Return to Question

make title about the code itself, not the objectives of code review ("_State what your code does in your title, not your main concerns about it._" - see https://codereview.stackexchange.com/questions/how-to-ask); update formatting,, add tag
Source Link

Does this look like bad design? Conways Game of Life in Java

Does this look like bad design?

Conways Game of Life in Java

You can see the whole bit here

Source Link

Does this look like bad design?

I created a program in Java that showcases Conway's Game of Life, in 4 days. Created the algorithm from scratch. However I got a scathing review and left me scratching my head. This was for a job application, the guy didn't say anything more, other than: "Kevin – we are going to pass. He has good documentation and coding hygiene, but his algorithms and design choices are questionable. It really does not make sense based on his background." I submitted it to Codacity and got a B based on style and such, I mean given the short time I had, I chose having more features, than elegant code. Could someone help me to understand what I did that was so bad.

You can see the whole bit here: https://github.com/kevingnet/GameOfLife/tree/master/GameOfLife

/* (non-Javadoc)
 * @see Cells#step()
 * Calculate next generation of cells
 * Algorithm:
 * - get both cells from the two buffers, both have the exact same location
 * - count live neighbors, regardless of state, somewhat optimal depending on state
 * - apply game rules: liveCell (2 or 3 liveNeighbors) = live, deadCell (3 liveNeighbors) = live
 * - clear cell buffer, by killing all cells
 * - swap buffers, readying for next step
 */
@Override
public void step() {
 for (int i = 0; i < this.cells.size(); ++i) {
 Cell cp = this.cells.get(i);
 Cell bcp = this.cellsBuffer.get(i);
 // count neighbor states, location is cached as adjacent cells contain references
 int liveNeighborsCount = getNeighborCount(cp);
 if (cp.isAlive()) {
 if (liveNeighborsCount == 2 || liveNeighborsCount == 3) {
 bcp.revive();
 }
 } else if (cp.isDead()) {
 if (liveNeighborsCount == 3) {
 bcp.revive();
 }
 }
 }
 // clear grid = kill all cells
 // we don't kill them before, since it would cause neighbor miscalculations
 for (Cell cp : this.cells) {
 cp.kill();
 }
 // swap arrays for next iteration
 ArrayList<Cell> tmp = this.cellsBuffer;
 this.cellsBuffer = this.cells;
 this.cells = tmp;
}
/**
 * Recalculate engine cell grid
 * use double buffer, so as to avoid unnecessary memory allocation and deallocation
 * Algorithm:
 * - save current alive cells into array
 * - resize cell buffer and add cells set to dead initially
 * - for each cell save neighbor locations
 * - clone array, both contain only dead cells
 * - copy saved alive cells into cell array
 * - save current dimensions
 * - restored saved cells into resized array
 */
private void recalculate() {
 // save alive cells
 ArrayList<Cell> alive = new ArrayList<>(0);
 for (Cell nc : this.cells) {
 if (nc.isAlive()) {
 alive.add(nc);
 }
 }
 int c = this.dimensions.width;
 int r = this.dimensions.height;
 this.cellsBuffer = new ArrayList<>(c * r);
 // initialize array, identity is set to dead, so first step is recalculated
 Cell cp = null;
 Point p = null;
 for (int i = 0; i < r; ++i) {
 for (int j = 0; j < c; ++j) {
 p = new Point(i, j);
 cp = new Cell(p);
 this.cellsBuffer.add(cp);
 }
 }
 // calculate and set neighbors
 if (this.teleport) {
 calculateNeighborsStiched();
 } else {
 calculateNeighbors();
 }
 
 //clone cell array
 this.cells = new ArrayList<>(c * r);
 for (Cell bcp : this.cellsBuffer) {
 this.cells.add(new Cell(bcp));
 }
 this.columns = c;
 this.rows = r;
 // copy old to current
 for (Cell ocp : alive) {
 Point op = ocp.getLocation();
 cp = this.cells.get((op.x * this.columns) + op.y);
 cp.revive();
 }
}
//cells are confined to the grid without teleporting capability
private void calculateNeighbors() {
 int c = this.dimensions.width;
 int r = this.dimensions.height;
 int col = 0;
 int row = 0;
 for (Cell ncp : this.cellsBuffer) {
 Point np = ncp.getLocation();
 int i = np.x;
 int j = np.y;
 ArrayList<Point> neighbors = new ArrayList<>(8);
 // go around the cell...
 // top
 row = i - 1;
 col = j;
 if (row >= 0) {
 addNeighbor(ncp, row, col, c, neighbors);
 }
 // bottom
 row = i + 1;
 col = j;
 if (row < r) {
 addNeighbor(ncp, row, col, c, neighbors);
 }
 // top left
 row = i - 1;
 col = j - 1;
 if (col >= 0 && row >= 0) {
 addNeighbor(ncp, row, col, c, neighbors);
 }
 // top right
 row = i - 1;
 col = j + 1;
 if (col < c && row >= 0) {
 addNeighbor(ncp, row, col, c, neighbors);
 }
 // bottom left
 row = i + 1;
 col = j - 1;
 if (col >= 0 && row < r) {
 addNeighbor(ncp, row, col, c, neighbors);
 }
 // bottom right
 row = i + 1;
 col = j + 1;
 if (col < c && row < r) {
 addNeighbor(ncp, row, col, c, neighbors);
 }
 // left
 row = i;
 col = j - 1;
 if (col >= 0) {
 addNeighbor(ncp, row, col, c, neighbors);
 }
 // right
 row = i;
 col = j + 1;
 if (col < c) {
 addNeighbor(ncp, row, col, c, neighbors);
 }
 ncp.setNeighbors(neighbors);
 }
}
lang-java

AltStyle によって変換されたページ (->オリジナル) /