I really like the DEAD_CELL and LIVING_CELL variables, they're a great approach to distinguishing cells without using just numbers. Here are some suggestions for the JavaScript:
Use modules for large scripts While writing everything in a single .js file can work fine in smaller scripts, when it gets to be more than a couple hundred lines long, it can start to get more difficult to manage than is ideal. For example, let's say you realize that cells aren't growing and dying as is intended. Currently, to debug it, unless you've memorized the function name, you'd probably scroll through the (long) code until you found a section that looks related, then examine it to see if it's really what you were looking for. That's not a very scalable approach.
It would be better if you could do something like go into a grid folder, inside which you can see different files containing functions related to the grid, and then you could browse it and go to the one file that's causing the issue, applyRules.js, and fix it up. Modules make large scripts a whole lot easier to write, maintain, and debug. Other benefits include explicit dependencies and very narrow scope (narrow scope helps a whole lot).
Use something like Webpack to coalesce all the separate .js files into a single one to serve on your HTML. Using a build process like this will also help you in other ways:
- Easy, automatic transpilation: you'll be able to write in as modern syntax as you want, and automatically transpile your code down to ES5 for production (allowing older browsers to understand your code, while keeping your source code modern and concise)
- Easy, automatic minification - A bit similar to above, reduces network payloads
Declare variables close to where they'll be used The thought process of someone reading the following might get somewhat disjointed - you make a row and col variable, and then return and don't use them if a condition is met.
document.getElementById('canvas').addEventListener('click', function(e) {
// Which box did user click on?
const row = Math.floor(e.offsetX / RES);
const col = Math.floor(e.offsetY / RES);
// Don't allow changes while simulation is running
if (isRunning) {
return;
}
// Case user clicked on a dead cell
if (grid[row][col] === DEAD_CELL) {
Consider returning as soon as possible instead, and only declaring the variables when you're going to use them in code below. Also consider renaming row and col to clickedRow and clickedCol for precision. This makes the code more self-documenting, and will mean that you can remove some of the comments:
document.getElementById('canvas').addEventListener('click', function(e) {
// Don't allow changes while simulation is running
if (isRunning) {
return;
}
const clickedRow = Math.floor(e.offsetX / RES);
const clickedCol = Math.floor(e.offsetY / RES);
if (grid[clickedRow][clickedCol] === DEAD_CELL) {
IDs You have a number of elements with IDs on the page. Unfortunately, such IDs become global variables. This has the potential to cause bugs due to variable name collisions. For example, you have speed and random IDs. What if, elsewhere, you tried to use a variable named speed or random, and/or accidentally refer to the standalone variables without doing querySelector or getElementById first? Then there could be problems that would have to be fixed (and could well be quite confusing).
While you could reduce the chance of collisions by changing the IDs to be less likely to be referred to accidentally (and by using a linter to warn you against the use of undefined variables), I’d personally prefer to avoid IDs entirely - either use classes instead to select elements:
<button class="clear">Clear</button>
const clearButton = document.querySelector(‘.clear’);
Or use a framework like React so that the elements are created at the same time as their handlers - that way, no elements have to be selected from the DOM at all. Just an idea - it makes the management of more complicated interfaces a bit easier.
generation The meaning of this variable wasn’t entirely clear to me until I saw how it was being used. Consider naming it something more precise like generationCount, or generationCountSinceLastChange.
updateGeneration: Throughout the code, you're frequently incrementing or setting to 0 the generation variable, and then calling updateGeneration, which does:
function updateGeneration() {
$(document).ready(function() {
$('#generation').text(generation);
});
}
Consider passing updateGeneration the new generation count instead:
function updateGeneration(newCount) {
generationCountSinceLastChange = newCount;
document.querySelector('#generation').textContent = newCount;
}
This lets you do updateGeneration(generationCountSinceLastChange + 1) or updateGeneration(0).
(This is the only place you're using jQuery. Feel free to remove it entirely - it isn't accomplishing anything useful. The document is already going to be loaded by the time this runs.)
Waiting for the window to load Rather than wrapping the entry point in a load listener, you may consider either giving the <script> tag the defer attribute or putting it at the bottom of the <body> - I think it's a bit easier to direct it from the HTML, rather than from the JS.
initialDraw Best to name functions based on what they do, rather than when they're called. This function looks to draws the border, so maybe call it drawBorder.
Avoid sloppy comparison with == and !=, they have strange coercion rules. Better to always use === and !==. Consider using ESLint and the rule eqeqeq.
DRYing
There are many places in the current code that are pretty repetitive.
On canvas click, rather than if elses, you can use the conditional operator and call updateGeneration at once:
updateGeneration(0);
const setAlive = grid[row][col] === DEAD_CELL;
grid[row][col] = setAlive ? LIVING_CELL : DEAD_CELL;
drawCell(setAlive ? 'white' : '2B823A', 'LightGray', 1, row * RES, col * RES, RES, RES);
createGrid Rather than calculating and passing createGrid the global constants every single time it's called:
const cols = WIDTH / RES;
const rows = HEIGHT / RES;
createGrid(cols, rows);
Consider having createGrid itself calculate the grid dimensions needed:
function createGrid() {
const cols = WIDTH / RES;
const rows = HEIGHT / RES;
You can do the same sort of thing for the draw function. (If the arguments to a function are always the same, that's an indication that it's something that should be handled inside the function, not something that gets passed to the function.)
applyRules can be refactored to avoid redundant checks. For example, given if ((neighbors < 2) || (neighbors > 3)) {, rather than testing against 2 and 3 for the other branch, you can use else. Or, even better, use the conditional operator.
for (let i = 1; i < cols - 1; i++) {
for (let j = 1; j < rows - 1; j++) {
const neighborCount = countNeighbors(i, j);
if (grid[i][j] === LIVING_CELL) {
// Living cells with less than two or more than 3 neighbors die
newArray[i][j] = (neighborCount < 2 || neighborCount > 3) ? DEAD_CELL : LIVING_CELL;
} else {
// Dead cells with exactly 3 neighbors are "re-born"
newArray[i][j] = neighborCount === 3 ? LIVING_CELL : DEAD_CELL;
}
}
}
Concise shape insertion When you have lots of shapes that can be inserted, having separate functions for each of them gets a bit repetitive. Consider making an object indexed by shape name instead, eg:
const patterns = {
lwss: [[0, 1], [-1, 1], [1, 1], [-2, 1], [-2, 0],
[2, 0], [-2, -1], [-1, -2], [2, -2]]
// ...
};
Then, rather than createLwss and all the other separate functions, you can do:
createPattern(patterns.lwss, x, y);
Avoid new Array, since it creates sparse arrays (which are only guaranteed to have a length property, but may or may not have own-properties from 0 up to the length) - if you forget to assign to every index of the array, iteration over it can be very weird. Consider using Array.from instead, or using .fill after calling new Array.
function createArray(cols, rows) {
return Array.from(
{ length: cols },
() => new Array(rows).fill(null)
);
}