Conway's Game of Life - Conventional JavaScript?

Question

I'm using the Game of Life Kata to help me learn JavaScript. I've picked up the syntax through Codecademy tutorials, but my current skill level is Novice.

The example code has working functionality for initialising a World with live cells and asking it to 'tick'.

All suggestions for improvement are welcome. I'd especially like feedback on the following:

• JavaScript idioms and conventions
• Function and variable scoping
• Making use of built-in functions and datatypes

myapp = {};
/**
 * World represents a 2 dimensional square matrix of cells.
 * @param dimension
 * @param liveCellCoordinates
 * @returns {myapp.World}
 */
myapp.World = function(dimension, liveCellCoordinatesArray) {
 this.dimension = dimension;
 this.liveCellCoordinates = liveCellCoordinatesArray;
};
/**
 * 
 * @returns {Array} of live cells after tick.
 */
myapp.World.prototype.tick = function() {
 var nextGenerationOfLiveCells = [];
// for each cell in world
for(var y=0; y<this.dimension; y++) {
 for(var x=0; x<this.dimension; x++) {
 var liveNeighbours = this.getLiveNeighbourCount(x, y);
 if(this.isCellAlive(x, y)) {
 if(isSurvivor(liveNeighbours)) {
 nextGenerationOfLiveCells.push([x,y]);
 } 
 }
 else {
 if(isBorn(liveNeighbours)) {
 nextGenerationOfLiveCells.push([x,y]);
 }
 }
 }
}
this.liveCellCoordinates = nextGenerationOfLiveCells;
return nextGenerationOfLiveCells;
};
myapp.World.prototype.getLiveNeighbourCount = function(x, y) {
var liveNeighbours = 0;
if(this.isCellAlive(x-1, y-1)) liveNeighbours++;
if(this.isCellAlive(x, y-1)) liveNeighbours++;
if(this.isCellAlive(x+1, y-1)) liveNeighbours++;
if(this.isCellAlive(x-1, y)) liveNeighbours++;
if(this.isCellAlive(x+1, y)) liveNeighbours++;
if(this.isCellAlive(x-1, y+1)) liveNeighbours++;
if(this.isCellAlive(x, y+1)) liveNeighbours++;
if(this.isCellAlive(x+1, y+1)) liveNeighbours++;
// jstestdriver.console.log(">>> liveNeighbours of ", x, y, ": ", liveNeighbours);
return liveNeighbours;
};
myapp.World.prototype.isCellAlive = function(x, y) {
 for(var cell in this.liveCellCoordinates) { 
 if(x === this.liveCellCoordinates[cell][0] && y === this.liveCellCoordinates[cell][1]) {
 return true;
 }
 }
 return false;
};
function isBorn(liveNeighbours) {
 return liveNeighbours === 3;
}
function isSurvivor(liveNeighbours) {
 return liveNeighbours === 2 || liveNeighbours === 3;
}

Answer 1

Looks quite good overall; not much to criticise. (I'm not looking at the overall structure or logic - just the style; the game of life can be written in so many ways)

Anyway, what I have is very trivial:

• Missing indentation in the bodies of tick and getLiveNeighbourCount
• Don't use a for...in loop for arrays (in isCellAlive); better and clearer to use a regular for-loop like you do elsewhere.
• You ought to define myapp as window.myapp. Right now it's implicitly global, because it's missing a var declaration, but you might as well make that explicit (or perhaps it doesn't need to be global at all?)
• Some more comments in the code itself would probably be nice.

Answer 2

myapp = {};

I think the general convention is to use camelCase, so myApp. But, why are you creating a namespace with only one property? And then later, with e.g. isBorn, you create functions in the global namespace? In any case, I would never use namepsaces named "myX" in any production code.

myapp.World.prototype.tick = function() {

Tick is a function!? I would expect tick to be a number, used by functions to know how often something has run etc.. Maybe update would be a better name?

for(var y=0; y<this.dimension; y++) {

To ease readability I would suggest moving the var keywords outside the loops. Better yet, declare them when the function opens. This will help you know which scope they are defined in.

I would use for (y = 0; y < dimension; y++) {. Note that dimension is no longer this.dimension. You should define var dimension = this.dimension at the top. Avoid unnecessary lookups where you can, especially for loops.

As for spacing in if constructs, there seem to be differing opinions. JSLint prefers if (this.isCellAlive(x, y)) {, jQuery prefers if ( this.isCellAlive(x, y) ) {.

 nextGenerationOfLiveCells.push([x,y]);

I think you should use objects instead of arrays. Arrays give you no benefit at all over objects. For example { x: x, y: y } instead of [x,y]

this.liveCellCoordinates = nextGenerationOfLiveCells;
return nextGenerationOfLiveCells;
};

This seems like an unnecessary side effect. Why set this.liveCellCoordinates if you are then going to return it? Maybe the function calling .tick() should be setting this.liveCellCoordinates or maybe this function should have a different return value? Something actually meaningful?

function isBorn(liveNeighbours) {
 return liveNeighbours === 3;
}
function isSurvivor(liveNeighbours) {
 return liveNeighbours === 2 || liveNeighbours === 3;
}

You're defining these 2 variables as globals. Considering your earlier attempt at namespacing I want to believe these should not be globals.

Better yet, enclose the entire code in self-invoking functions to avoid leaking these two functions to the global object:

var myApp = (function () {
 // Your code here
 return {
 World: World
 };
}());