8
\$\begingroup\$

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 < width; i++) {
        this.cells[i] = new Array(height);        
        for (var j = 0; j < height; j++) {
            this.cells[i][j] = new Cell(false, i, j);
        }
    }
    /**
     * an array of cells whos state is to be flipped at the end of a step.
     */
    this.cellsToFLip = new Array();
    /**
     * list of currently alive cells. for faster draw.
     */
    this.aliveCells = new Array();
}
Grid.prototype.revive = function(x, y) {
    if (x <= 0  || y <= 0 || x >= 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 < this.aliveCells.length; i++) { // remove from alive list
        if (this.aliveCells[i] == this.cells[x][y]) { // same cell?
            this.aliveCells.splice(i, 1); // remove cell
        }
    }
};
/**
 * sets all cells to dead.
 */
Grid.prototype.reset = function() {
    for (var i = 0; i < this.width; i++) {
        for (var j = 0; j < this.height; j++) {
            this.kill(i, j);
        }
    }
    this.iteration = 0;
};
/**
 * internal function. Marks the cell at given coordinates so it will be flipped
 * at the end of a step.
 */
Grid.prototype.markForFlip = function(cell) {
    this.cellsToFLip.push(cell);
};
/**
 * internal function. Flips the state of all cells (dead -> alive; alive -> dead) in cellsToFLip.
 */
Grid.prototype.applyFlip = function() {
    for (var i = 0; i < this.cellsToFLip.length; i++) {
        var currentCell = this.cellsToFLip[i];
        if (currentCell.isAlive) {
            this.kill(currentCell.x, currentCell.y);
        } else {
            this.revive(currentCell.x, currentCell.y);
        }
    }
};
Grid.prototype.step = function() {
    this.cellsToFLip = new Array();
    this.iteration++; // how often step was called

    for (var i = 1; i < this.width - 1; i++) { // we have a frame of 1 that is always dead (saves bound check)
        for (var j = 1; j < this.height - 1; j++) {
            var currentCell = this.cells[i][j];
            var aliveNeighborCount = 0;
            // check each adjacent cell
            for (var ii = i - 1; ii <= i + 1; ii++) {
                for (var jj = j - 1; jj <= j + 1; jj++) {
                    if (this.cells[ii][jj] != currentCell // don't count current cell
                            && this.cells[ii][jj].isAlive) {
                        aliveNeighborCount++;
                    }
                }
            }

            if (currentCell.isAlive
                    && (aliveNeighborCount < 2 || aliveNeighborCount > 3)) {
                // cell is alive and has less than 2 or more than 3 alive neighbors. 
                // kill it
                this.markForFlip(currentCell);

            } else if (!currentCell.isAlive && aliveNeighborCount === 3) {
                // cell is dead. revive it cell
                this.markForFlip(currentCell);
            }
        }
    }
    this.applyFlip();
};
Grid.prototype.toString = function() {
    var output = "";
    // print row by row
    for (var j = 0; j < this.height; j++) {
        for (var i = 0; i < this.width; i++) {
            if (this.cells[i][j].isAlive) {
                output += this.cells[i][j].toSymbol();
            } else {
                output += this.cells[i][j].toSymbol();
            }
        }
        output += "<br/>";
    }
    return output;
};

control.js:

"use strict";
/*jslint latedef:false*/
var cellHeight = 20; // px height and width of cell when drawn
var gridWidth = 30;  // width of grid in cells
var gridHeight = 20; // height of grid in cells
var updateTimeMs = 500;

var grid = new Grid(gridWidth, gridHeight);
var canvas = document.getElementById('out2');
var intervalId; // id of interval, used to clear it

loadSettings();

/*
 * load and apply settings. This resets the grid.
 */
function loadSettings() {
    // load settings
    updateTimeMs = document.getElementById('speed').value;
    gridHeight = document.getElementById('height').value;
    gridWidth = document.getElementById('width').value;
    cellHeight = document.getElementById('cellHeight').value;

    // apply settings
    grid = new Grid(gridWidth, gridHeight);

    canvas.width = gridWidth * cellHeight;
    canvas.height = gridHeight * cellHeight;
    reset();    
}

/**
 * call on mouse click or drag. Will spawn a cell at coordinates of mouse event
 * or kill a cell if shift is pressed.
 */
function input(event) {
    var coords = getMousePos(canvas, event);
    var x = toInteger(coords.x / cellHeight) - 1;
    var y = toInteger(coords.y / cellHeight) - 1;

    if (event.shiftKey) {
        if (grid.cells[x][y].isAlive) {
            grid.kill(x, y);
            draw(); // only draw on change
        } 
    } else {
        if (!grid.cells[x][y].isAlive) {
            grid.revive(x, y);
            draw(); // only draw on change
        }
    }
}

/**
 * get position of mouse event relative to given canvas.
 */
function getMousePos(canvas, event) {
    var rect = canvas.getBoundingClientRect();
    return {
        x: event.clientX - rect.left,
        y: event.clientY - rect.top
    };
}

function reset() {
    grid.reset();
    running = false;
    clearInterval(intervalId);
    draw();
}

var running = false;
var iterations = document.getElementById('out');
function start() {
    if (!running) {
        running = true;
        intervalId = setInterval(function() {
            grid.step();
            draw();
            iterations.innerHTML = grid.iteration;
        }, updateTimeMs);
    }
}

function pause() {
    running = false;
    clearInterval(intervalId);
}

function draw() {
    if (canvas.getContext) {
        drawCanvas(canvas);
    } else {
        drawSimple(grid);
    }
}

/* 
 * alternativ ascii draw method in case canvas is not supported.
 */
function drawSimple() {
    var div = document.getElementById('out').innerHTML = grid.toString();
}

/*
 * draws grid to given canvas.
 */
function drawCanvas(canvas) {
    var context = canvas.getContext('2d');
    context.clearRect(0, 0, canvas.width, canvas.height); // clear canvas
    context.strokeRect(0, 0, canvas.width, canvas.height); // draw frame
    for (var i = 0; i < grid.aliveCells.length; i++) {
        context.strokeRect(cellHeight * grid.aliveCells[i].x, cellHeight * grid.aliveCells[i].y,
                cellHeight, cellHeight);
    }
}

function toInteger(number) {
    return Math.ceil(number);
}

index.html:

<!DOCTYPE html>
<html dir="ltr" lang="en-US">
    <head>
        <title>Game of Life</title>
        <meta charset="UTF-8">
    </head>
    <body>
        <div id="output">
            <div id="out"></div>
            <canvas id="out2" width="200" height="200" onclick="input(event);" 
                    onmouseup="down = 0;" onmousedown="down = 1;" 
                    onmousemove="if (down == 1) input(event);"></canvas>
            <div id="iterations"></div>
        </div>

        <fieldset class='controls'>
            <legend>Controls</legend> 
            <p>Click or Drag to draw pattern, use Shift to undo.</p>
            <input id="start" type="button" value="Start" onclick="start();" />
            <input id="pause" type="button" value="Pause" onclick="pause();" />
            <input id="reset" type="button" value="Reset" onclick="reset();" />
        </fieldset>

        <fieldset class='settings'> 
            <legend>Settings</legend> 
            <form> 
                <label for="width">Game Area Width (cells):</label> 
                <input type="range" id="width" name="width" min="1" max="300" step="1" value="30" onmousemove="update(this.value, 'lwidth')"> 
                <output for="width" id="lwidth">30</output> 
            </form> 
            <form> 
                <label for="height">Game Area Height (cells):</label> 
                <input type="range" id="height" name="height" min="1" max="300" step="1" value="20" onmousemove="update(this.value, 'lheight')"> 
                <output for="height" id="lheight">20</output> 
            </form> 
            <form> 
                <label for="cellHeight">Cell Height (px):</label> 
                <input type="range" id="cellHeight" name="cellHeight" min="1" max="50" step="1" value="20" onmousemove="update(this.value, 'lcellHeight')"> 
                <output for="cellHeight" id="lcellHeight">20</output>
            </form> 
            <form> 
                <label for="speed">Update speed (ms):</label> 
                <input type="range" name="speed" id="speed" min="10" max="5000" step="10" value="500" onmousemove="update(this.value, 'lspeed')"> 
                <output for="speed" id="lspeed">500</output> 
            </form> 
            <input type="submit" value="Apply & Reset" onclick="loadSettings();"> 
        </fieldset>

        <script type="text/javascript">
          function update(value, id) {
            document.getElementById(id).innerHTML = value; 
          }
        </script>
        <script type="text/javascript" src="gameLogic.js"></script>
        <script type="text/javascript" src="control.js"></script>
    </body>
</html>
\$\endgroup\$
  • 1
    \$\begingroup\$ Side-note: new Array is discouraged, to say the least. frowned upon is probably more accurate. Check MDN docs for Object.defineProperty, too, to ensure your objects will work as expected, too \$\endgroup\$ – Elias Van Ootegem Aug 5 '14 at 12:34
  • \$\begingroup\$ @Elias As far as I'm aware, new Array(length) is the one form of Array() that is not frowned upon, simply because it's the most natural way to declare an Array of a fixed size. \$\endgroup\$ – Schism Aug 5 '14 at 13:51
  • 3
    \$\begingroup\$ @Schism: Given that JS arrays don't have a fixed size, I wouldn't call that a decisive argument. The arrays are being initialized, so there's no need to construct an array full of undefined's in the constructor. That's just my opinion, though \$\endgroup\$ – Elias Van Ootegem Aug 5 '14 at 14:51
  • \$\begingroup\$ I have seen new Array(n).join(c) used to create variable-length character padding. That's about the only legitimate use of new Array I can think of. \$\endgroup\$ – Dagg Aug 6 '14 at 20:02
5
\$\begingroup\$
  • 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.

\$\endgroup\$
  • \$\begingroup\$ thanks, that are a lot of helpful points. 1. the capital l is indeed a typo 2. markForFlip exists because it somewhat separates the flip logic from the step logic (I should probably also move cellsToFLip into applyFlip), in case I want to implement it differently later on (for example in a way that matches my comment^^) 3. yes, some comments are probably unnecessary. but same cell? is there because my IDE shows a warning when using == to compare. The comment is there to indicate that this is not a typo \$\endgroup\$ – tim Aug 5 '14 at 14:42

Your Answer

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

Not the answer you're looking for? Browse other questions tagged or ask your own question.