patternjavascriptMinor
Game of Life - OOP and Best Practices
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
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
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
-
It seems to me like you mis-capitalised
-
Personally, I would put the neighbour counting in a separate function from
-
As @EliasVanOotegem mentioned,
-
In your
-
I am very much not a fan of using
-
There is such a thing as too simple a function. I don't see much point in
-
There's also such a thing as too many comments. Comments like
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.