HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavascriptMinor

Game of Life - OOP and Best Practices

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
andgamepracticeslifeoopbest

Problem

I always have a problem figuring out what good JavaScript coding practices and conventions are and if I'm following them, especially concerning OOP. So I implemented The Game of Life and wanted to ask if you have any criticism or suggestions.

My main concern is the gameLogic.js file, but for completeness I'll post all my code (if you see something that can be improved in the other files, I'm happy to hear about it as well).

I'm not really concerned about performance, only code structure, etc.

gameLogic.js:

```
"use strict";
/jslint latedef:false/
function Cell(isAlive, x, y) {
this.isAlive = false;
this.x = x;
this.y = y;
}
/**
* returns a symbol representing the current state of this cell.
*
* @returns {String}
*/
Cell.prototype.toSymbol = function() {
return this.isAlive ? "X" : "_";
};

/**
* the grid.
* start coordinates are (0,0) in the upper left corner (y-axis goes down from here).
* The grid has a frame (width: 1 cell) that is always dead.
*
* The value of a cell is either dead or alive.
* The grid will be initialized with all cells set to dead.
*
* @param {type} width
* @param {type} height
* @returns {Grid}
*/
function Grid(width, height) {
this.width = width;
this.height = height;
this.iteration = 0;
this.cells = new Array(width);
// init cells with 0.
for (var i = 0; i = this.width - 1 || y >= this.height - 1) {
return; // frame of 1 dead cell
}
this.cells[x][y].isAlive = true;
this.aliveCells.push(this.cells[x][y]); // add to alive list
};
Grid.prototype.kill = function(x, y) {
this.cells[x][y].isAlive = false;
for (var i = 0; i alive; alive -> dead) in cellsToFLip.
*/
Grid.prototype.applyFlip = function() {
for (var i = 0; i 3)) {
// cell is alive and has less than 2 or more than 3 alive neighbors.
// kill it
this.markForFlip(currentCell);

} else if (!currentCell.isAlive && aliveNeighborCo

Solution

-
In general, you should not be putting "use strict"; in global scope; this mistake has been made by large companies like Amazon and Intel. In your specific case, since you can guarantee all your code will be strict, then it's not as big of a deal.

-
It seems to me like you mis-capitalised cellsToFlip once and your IDE's auto-complete made you not catch it... unless there's a specific reason the third l is capitalised?

-
Personally, I would put the neighbour counting in a separate function from Grid.step().

-
As @EliasVanOotegem mentioned, new Array() is frowned upon. Replace this with this.cellsToFlip = [];.

-
In your Grid.kill() function, you loop through your entire list before splicing. You can simplify this with indexOf like so:

Grid.prototype.kill = function(x, y) {
    this.cells[x][y].isAlive = false;
    this.aliveCells.splice(this.aliveCells.indexOf(this.cells[x][y]), 1);
}


-
I am very much not a fan of using Grid.toString() like this. This logic belongs more naturally directly in the drawSimple() function.

-
There is such a thing as too simple a function. I don't see much point in Grid.markForFlip() existing at all, since it doesn't contain any logic. It also doesn't do what the comment claims it does.

-
There's also such a thing as too many comments. Comments like add to alive list and same cell? add no value.

Code Snippets

Grid.prototype.kill = function(x, y) {
    this.cells[x][y].isAlive = false;
    this.aliveCells.splice(this.aliveCells.indexOf(this.cells[x][y]), 1);
}

Context

StackExchange Code Review Q#59093, answer score: 5

Revisions (0)

No revisions yet.