I am currently reading Eloquent JavaScript and am trying to solve the exercise problems. One of the exercises in Chapter 18 asks to emulate Conway's Game of Life using checkboxes in JavaScript.
Before looking at the official solution, I like to solve the exercise on my own and then compare it with the author's solution. This time, though, I am a little confused.
My solution:
<html>
<head>
<title>Conway's game of life</title>
<style type="text/css">
h1 {
margin-top : 50px;
font-family : Cardo;
}
#world{
width : 800px;
height : 400px;
margin : 0 auto;
margin-top :100px;
padding : 0px;
}
input[type="checkbox"] {
margin : 0px;
margin-top : 5px;
margin-left : 5px;
}
</style>
</head>
<body>
<center>
<h1>Conway's game of life</h1>
</center>
<div id="world">
</div>
<script type = "text/javascript">
'use strict';
// World is represented by a div and cells in the world are represented by checkboxes
// As defined by the browser default styles at zoom level of 100%
var WIDTH_CELL = 17;
var HEIGHT_CELL = 17;
// Div that holds the world of the cells
var world = document.querySelector("#world");
var HEIGHT_WORLD = world.getBoundingClientRect().height;
var WIDTH_WORLD = world.getBoundingClientRect().width;
var NUM_ROWS = Math.floor(HEIGHT_WORLD / HEIGHT_CELL);
var NUM_COLS = Math.floor(WIDTH_WORLD / WIDTH_CELL);
var NUM_CELLS = NUM_ROWS * NUM_COLS;
// Holds every cell element that exists in the DOM
var cells = [];
// Fill the world with cells
for (var i = 0 ; i < NUM_CELLS ; i++) {
var newCell = getNewCell();
addNewCellToWorld(world, newCell);
cells.push(newCell);
}
/*
The rules are as follows
1 . Any live cell with fewer than two or more than three live neighbors dies
2 . Any live cell with two or three live neighbors lives on to the next generation
3 . Any dead cell with three live neighbors becomes alive
*/
function incrementGeneration() {
var cellsThatLive = [];
var cellsThatDie = [];
for(var currentCell = 0 ; currentCell < cells.length ; currentCell++){
var numLiveNeighbors = getLiveNeighborsCount(currentCell);
if(isAlive(currentCell)) {
if(numLiveNeighbors < 2 || numLiveNeighbors > 3)
cellsThatDie.push(currentCell);
else if(numLiveNeighbors === 2 || numLiveNeighbors === 3)
cellsThatLive.push(currentCell);
}else{
if(numLiveNeighbors === 3)
cellsThatLive.push(currentCell);
}
}
for(var i = 0 ;i < cellsThatLive.length ; i++)
cells[cellsThatLive[i]].checked = true;
for(var j = 0 ;j < cellsThatDie.length ; j++)
cells[cellsThatDie[j]].checked = false;
}
setInterval(function(){
incrementGeneration();
}, 150);
// Vector that holds the position of the cell in form of x and y coordinates in the world
function Vector(x, y) {
this.x = x;
this.y = y;
}
Vector.prototype.add = function(otherVector) {
return (new Vector(this.x + otherVector.x, this.y + otherVector.y));
}
Vector.prototype.subtract = function(otherVector) {
return (new Vector(this.x - otherVector.x, this.y - otherVectory.y));
}
var neighborDirections = {
"n" : new Vector(0, 1),
"e" : new Vector(1, 0),
"s" : new Vector(0, -1),
"w" : new Vector(-1, 0),
"ne" : new Vector(1, 1),
"nw" : new Vector(-1, 1),
"se" : new Vector(1, -1),
"sw" : new Vector(-1, -1)
};
/*
* Function : getLiveNeighborsCount(one dimensional index of the cell whose neighbors are to be found)
* ------------------------------------------------------------------------------------------
* Returns the neighboring (directly touching even the diagonal ones) cells of the cell whose
* neighbors are asked for.
* Returns the number of live neighbors the current cell under consideration has
*/
function getLiveNeighborsCount(cellIndex) {
var neighbors = [];
var currentCellVector = getVectorFromIndex(cellIndex);
for(var direction in neighborDirections) {
addNeighborIfValid(neighbors, currentCellVector.add(neighborDirections[direction]));
}
return neighbors.length;
}
/*
* Function : addNeighborIfValid(list of neighbors(array), the vector containing position of the passed neighbor)
* --------------------------------------------------------------------------------------------------------------
* Checks if the passed in neighbor is a valid neighbor. A valid neighbor is one which is inside the bounds of
* the world and not outside of it and the one that is alive is only considered a neighbor.
* If the neighbor is a valid one then adds it to the list of neighbors and does nothing otherwise
*/
function addNeighborIfValid(neighborsList, neighborVector) {
if(isInBounds(neighborVector) && isAlive(getIndexFromVector(neighborVector)))
neighborsList.push(getIndexFromVector(neighborVector));
}
function isAlive(cellIndex) {
return (cells[cellIndex].checked);
}
/*
* Function : isInBounds(vector on which the check is to be applied)
* -----------------------------------------------------------------
*
*/
function isInBounds(vector) {
return ((vector.x >= 0 && vector.x <= NUM_COLS - 1) && (vector.y >= 0 && vector.y <= NUM_ROWS - 1));
}
/*
* Function : getVectorFromIndex(index of the element in the array i.e flatIndex)
* --------------------------------------------------------------------------------
* Converts a simple flat index into coordinates in two dimensions and returns a
* vector object of with those parameters.
*/
function getVectorFromIndex(flatIndex) {
var xCoord = flatIndex % NUM_COLS;
var yCoord = (flatIndex - xCoord) / NUM_COLS;
return (new Vector(xCoord, yCoord));
}
/*
* Function : getIndexFromVector(vector which is to be converted to a one dimensional index)
* -----------------------------------------------------------------------------------------
*/
function getIndexFromVector(vector) {
return (NUM_COLS * vector.y + vector.x);
}
/*
* Function : addNewCellToWorld(div element that acts like world, checkbox element that acts as a cell)
* -----------------------------------------------------------------------------------------------------
*/
function addNewCellToWorld(world, cell) {
world.appendChild(cell);
}
/*
* Function : getNewCell()
* Usage : world.appendChild(newCell());
* --------------------------------------------
* Creates and returns a checkbox element that
* is considered as a cell in the program.
* Returns on random, checked or unchecked cell
* checked means alive and unchecked means dead
*/
function getNewCell() {
var newCell = document.createElement("input");
newCell.type = "checkbox";
newCell.checked = (Math.random() > 0.5)
return newCell;
}
</script>
</body>
</html>
Author's solution (do not review this):
<!doctype html>
<script src="code/promise.js"></script>
<div id="grid"></div>
<button id="next">Next generation</button>
<button id="run">Auto run</button>
<script>
var width = 30, height = 15;
// I will represent the grid as an array of booleans.
var gridNode = document.querySelector("#grid");
// This holds the checkboxes that display the grid in the document.
var checkboxes = [];
for (var y = 0; y < height; y++) {
for (var x = 0; x < width; x++) {
var box = document.createElement("input");
box.type = "checkbox";
gridNode.appendChild(box);
checkboxes.push(box);
}
gridNode.appendChild(document.createElement("br"));
}
function gridFromCheckboxes() {
return checkboxes.map(function(box) { return box.checked; });
}
function checkboxesFromGrid(grid) {
return grid.forEach(function(value, i) { checkboxes[i].checked = value; });
}
function randomGrid() {
var result = [];
for (var i = 0; i < width * height; i++)
result.push(Math.random() < 0.3);
return result;
}
checkboxesFromGrid(randomGrid());
// This does a two-dimensional loop over the square around the given
// x,y position, counting all fields that have a cell but are not the
// center field.
function countNeighbors(grid, x, y) {
var count = 0;
for (var y1 = Math.max(0, y - 1); y1 <= Math.min(height, y + 1); y1++) {
for (var x1 = Math.max(0, x - 1); x1 <= Math.min(width, x + 1); x1++) {
if ((x1 != x || y1 != y) && grid[x1 + y1 * width])
count +=1 ;
}
}
return count;
}
function nextGeneration(grid) {
var newGrid = new Array(width * height);
for (var y = 0; y < height; y++) {
for (var x = 0; x < width; x++) {
var neighbors = countNeighbors(grid, x, y);
var offset = x + y * width;
if (neighbors < 2 || neighbors > 3)
newGrid[offset] = false;
else if (neighbors == 2)
newGrid[offset] = grid[offset];
else
newGrid[offset] = true;
}
}
return newGrid;
}
function turn() {
checkboxesFromGrid(nextGeneration(gridFromCheckboxes()));
}
document.querySelector("#next").addEventListener("click", turn);
var running = null;
document.querySelector("#run").addEventListener("click", function() {
if (running) {
clearInterval(running);
running = null;
} else {
running = setInterval(turn, 400);
}
});
</script>Is my solution unnecessarily long? Does that make it more confusing?
-
\$\begingroup\$ I've been "about to start reviewing" for like 10 minutes. but I need to keep my little checkboxes alive! \$\endgroup\$blaizor– blaizor2015年04月19日 13:00:06 +00:00Commented Apr 19, 2015 at 13:00
1 Answer 1
No, your implementation is not gratuitously complicated.
Most of what you wrote extra is going to pay off if you ever want to extend the app (to use other rules, other sorts of neighborhoods, cell types and so on). You also provided an extra Vector class in there which isn't per se part of Game of Life - you can pretend it comes as a separate math library. There are however bits and pieces that can be simplified.
The cellsThatLive/Die are not really necessary. Compare your incrementGeneration with the author's nextGeneration. The function should't take the old grid from the outer scope, but rather as a parameter - this allows you to use the function separately elsewhere, in another program or from a unit test. Also, I'd separate the data from the presentation - the function that computes the next generation should have nothing to do with html elements; it should only move data around.
This
setInterval(function(){
incrementGeneration();
}, 150);
can be rewritten simply as setInterval(incrementGeneration, 150);; there's no need for the extra function wrap.
You have a bunch of for loops that can be simplified if you use forEach, map and the like (notice the author uses them). Old-school for loops contain a LOT of code/extra information: you declare a variable, you initialize it, you do this and that when you actually just want to iterate.
The getLiveNeighborsCount function creates an array neighbors but you only use it to retrieve its length - you can just count directly. Oh and It's bad practice to mutate objects passed as arguments (I'm talking about the addNeighborIfValid function (mutating most things should be considered bad practice IMO)).
neighborDirections can be a simple array since you're not doing anything with the keys.
Update
Regarding modifying (mutating) parameters, it's best avoided. It's easier to reason about functions if you know they just LOOK at the data they are presented with and do not TOUCH it - the function can of course return a value and you decide what to do with it.
This is a common pattern that I see a lot:
// some function that loads an image given an URL
// and also may take a bunch of options { format, resize and whatnot }
function loadImage(url, options) {
// options is optional itself
options = options || {};
// options have defaults
// and this is where the "crime" occurs
options.format = options.format || 'RGB';
options.resizeX = options.resizeX || 640;
options.resizeY = options.resizeY || 480;
// load the image and use options
var image = new Image() ...
image.src = url ...
resize(options.resizeX, options.resizeY...
}
If I call this function loadImage('blaha.jpg', options) I'm now going to get my image, but I'm also going to get my original options object contaminated with stuff that I never asked for - these are unintended side effects.
Also, take a look at a related question: https://softwareengineering.stackexchange.com/questions/245767/is-it-an-antipattern-modifying-an-incoming-parameter
-
\$\begingroup\$ Thanks for reviewing. First of all I was not aware of the alternate setInterval syntax so thanks a bunch for mentioning that. Coming from C style languages I tend to forget about the awesome helper functions like map, reduce, forEach e.t.c I sure will utilise them from now on. The function getLiveNeighborsCount was earlier getLiveNeighbors therefore it contains the remains of it, will be cleaning that out. neighborDirections could definitely be made into an array but I decided to keep it that way for better readability. It'll be really helpful if you can elaborate more on mutating objects part \$\endgroup\$Nash Vail– Nash Vail2015年04月21日 05:33:30 +00:00Commented Apr 21, 2015 at 5:33