5
\$\begingroup\$

I would like to kindly ask others to review my code. I'd like suggestions, criticisms, and discussions on what is good and what could be done better.

const canvas = document.querySelector('.gameOfLife');
const ctx = canvas.getContext('2d');

// create an array
const canvasWidth = 100;
const canvasHeight = 100;
canvas.width = 3 * canvasWidth;
canvas.height = 3 * canvasHeight;
let gameBoard = createBoard(canvasWidth, canvasHeight);
let gameBoard2 = createBoard(canvasWidth, canvasHeight);
let gameBoard3 = createBoard(canvasWidth, canvasHeight);

function createBoard(width, height) {
  let board = new Array(0);
  for (let i = 0; i < width; i++) {
    board [i] = [];
    for (let j = 0; j < height; j++){
      board[i][j] = 0;
    }
  }
  return board;
}

function randomNumber(max) {
  return Math.floor(Math.random() * max);
}

function randomPos() {
  const x = randomNumber(canvasWidth);
  const y = randomNumber(canvasHeight);
  return [x, y];
}

function randomBoard(arr) {
  for (let i = 0; i <= 0.2 * canvasHeight * canvasWidth; i++) {
    const [x, y] = randomPos();
    arr[x][y] = 1;
  }
}

function drawBoard(arr) {
  ctx.clearRect(0, 0, canvas.width, canvas.height)
  for (let i = 0; i < canvasWidth; i++) {
    for (let j = 0; j < canvasHeight; j++){
      if (arr[i][j] === 1) {
        ctx.fillStyle = "#012345";
        ctx.fillRect(i*3, j*3, 3, 3);
      }
    }
  }
}

// calculate new cycle

function checkSurrounding(x, y) {
  let sum = 0;

  startX = x-1 < 0 ? x : x-1;
  stopX = x+1 >= canvasWidth ? x : x+1;
  startY = y-1 < 0 ? y : y-1;
  stopY = y+1 >= canvasHeight ? y : y+1;

  for (let i = startX ; i <= stopX; i++) {
    for (let j = startY ; j <= stopY; j++) {
      if (i !== x || j !== y) {
        sum += gameBoard[i][j];
      }
    }
  }

  return sum;
}

function deadOrAlive(x, y) {
  const surround = checkSurrounding(x, y);

  if (gameBoard[x][y] === 1 && (surround === 2 || surround === 3)) return 1;
  else if (gameBoard[x][y] === 0 && surround === 3) return 1;
  else return 0;
}

function nextGene() {
  let newBoard = createBoard(canvasWidth, canvasHeight);

  for (let i = 0; i < canvasWidth; i++) {
    for (let j = 0; j < canvasHeight; j++) {
      newBoard[i][j] = deadOrAlive(i, j);
    }
  }

  gameBoard = newBoard;
  drawBoard(newBoard);

}


randomBoard(gameBoard);
drawBoard(gameBoard);

setInterval(nextGene, 150);
<!DOCTYPE html>
<html lang="en">
<head>
  <meta charset="UTF-8">
  <title>Game of Life - Canvas</title>
</head>
<body>
  <canvas class="gameOfLife" width="400" height="400"></canvas>
  <script type="text/javascript" src="scripts.js"></script>
</body>

<style>
  body {
    background-color: #ffc600;
  }
  .gameOfLife{
    margin: 15px auto;
    background-color: #ff6c00;
    display: block;
  }
</style>
</html>

Improve this question
\$\endgroup\$
1
  • \$\begingroup\$ Any time I see someone writing a Game Of Life program I always suggest: implement Gosper's Algorithm. You will learn a lot about the power of purely functional data structures and memoization. Once you can compute trillions of generations a second on a board with trillions of cells, naive implementations seem trivial. \$\endgroup\$
    – Eric Lippert
    Aug 29, 2017 at 17:39

2 Answers 2

Reset to default
3
\$\begingroup\$

I have gone over the code briefly and looked at a couple optimizations, but haven't come up with any drastic changes. I will try to go over it again when I find time.

What is good

I like how the various functions are generally short. And the CSS/HTML is very simple too! It is a nice use of the canvas.

Suggestions

Iterations

Anytime you iterate over an array using a for loop, consider using a functional approach like .forEach(), .map(). That way, you won't have to handle the bookkeeping of iterating the variable (e.g. var i) and indexing into the array. For more information, I recommend going through these exercises. Though be aware that functional approaches are not always optimal/faster.

Creating board:

Inspired by the comment from Longfei Wu on this question, the function to create the board could take on a functional approach using Array.fill() and Array.map()

function createBoard(width, height) {
  return Array(width).fill(0).map(function() {
    return Array(height).fill(0);
  });
}

And if you are open to using ES-6 arrow functions, that can be even shorter:

function createBoard(width, height) {
  let board = Array(width).fill(0).map(() => Array(height).fill(0));
  return board;
}

See comparison in this jsPerf. When I ran it in Chrome the array fill-map approach was faster, though the opposite was true in Firefox, Mobile Safari and emphasized textEdge.

Drawboard:

.forEach() can be used here instead of the for statements:

function drawBoard(arr) {
  ctx.clearRect(0, 0, canvas.width, canvas.height)
  arr.forEach(function(innerArray, i) {//  for (let i = 0; i < canvasWidth; i++) {
    innerArray.forEach(function(cell, j) {//for (let j = 0; j < canvasHeight; j++){
      if (cell === 1) {
        ctx.fillStyle = "#012345";
        ctx.fillRect(i*3, j*3, 3, 3);
      }
    });
  });
}

And the same is true for nextGene().

deadOrAlive()

Because checkSurrounding() checks 3-9 spaces around the given cell, I see cases where it returns values greater than 3. Should deadOrAlive() have logic in those cases? It appears that it only has logic for return values of 2 and 3.

Updated snippet:

const canvas = document.querySelector('.gameOfLife');
const ctx = canvas.getContext('2d');

// create an array
const canvasWidth = 100;
const canvasHeight = 100;
canvas.width = 3 * canvasWidth;
canvas.height = 3 * canvasHeight;
let gameBoard = createBoard(canvasWidth, canvasHeight);
let gameBoard2 = createBoard(canvasWidth, canvasHeight);
let gameBoard3 = createBoard(canvasWidth, canvasHeight);

function createBoard(width, height) {
  return Array(width).fill(0).map(function() {
    return Array(height).fill(0);
  });
}

function randomNumber(max) {
  return Math.floor(Math.random() * max);
}

function randomPos() {
  const x = randomNumber(canvasWidth);
  const y = randomNumber(canvasHeight);
  return [x, y];
}

function randomBoard(arr) {
  for (let i = 0; i <= 0.2 * canvasHeight * canvasWidth; i++) {
    const [x, y] = randomPos();
    arr[x][y] = 1;
  }
}

function drawBoard(arr) {
  ctx.clearRect(0, 0, canvas.width, canvas.height)
  arr.forEach(function(innerArray, i) {//  for (let i = 0; i < canvasWidth; i++) {
    innerArray.forEach(function(cell, j) {//for (let j = 0; j < canvasHeight; j++){
      if (cell === 1) {
        ctx.fillStyle = "#012345";
        ctx.fillRect(i*3, j*3, 3, 3);
      }
    });
  });
}

// calculate new cycle

function checkSurrounding(x, y) {
  let sum = 0;

  startX = x-1 < 0 ? x : x-1;
  stopX = x+1 >= canvasWidth ? x : x+1;
  startY = y-1 < 0 ? y : y-1;
  stopY = y+1 >= canvasHeight ? y : y+1;

  for (let i = startX ; i <= stopX; i++) {
    for (let j = startY ; j <= stopY; j++) {
      if (i !== x || j !== y) {
        sum += gameBoard[i][j];
      }
    }
  }

  return sum;
}

function deadOrAlive(x, y) {
  const surround = checkSurrounding(x, y);

  if (gameBoard[x][y] === 1 && (surround === 2 || surround === 3)) return 1;
  else if (gameBoard[x][y] === 0 && surround === 3) return 1;
  else return 0;
}

function nextGene() {
  let newBoard = createBoard(canvasWidth, canvasHeight);

  newBoard.forEach(function(innerArray, i) {//  for (let i = 0; i < canvasWidth; i++) {
    innerArray.forEach(function(cell, j) {//for (let j = 0; j < canvasHeight; j++){
      newBoard[i][j] = deadOrAlive(i, j);
    });
  });

  gameBoard = newBoard;
  drawBoard(newBoard);

}


randomBoard(gameBoard);
drawBoard(gameBoard);

setInterval(nextGene, 150);
<!DOCTYPE html>
<html lang="en">
<head>
  <meta charset="UTF-8">
  <title>Game of Life - Canvas</title>
</head>
<body>
  <canvas class="gameOfLife" width="400" height="400"></canvas>
  <script type="text/javascript" src="scripts.js"></script>
</body>

<style>
  body {
    background-color: #ffc600;
  }
  .gameOfLife{
    margin: 15px auto;
    background-color: #ff6c00;
    display: block;
  }
</style>
</html>

Improve this answer
\$\endgroup\$
4
  • \$\begingroup\$ I like the way you create the board and interate thru. I will use this in my future projects. At the begining the x in arrow function was unclear for me: let board = Array(width).fill(0).map(x => Array(height).fill(0)); you can remove x from arrow function and use just () => Ar... \$\endgroup\$
    – korczas
    Aug 29, 2017 at 4:35
  • \$\begingroup\$ cool -yeah empty parentheses is the simplest form, since there are no parameters. I have updated my answer for that section. \$\endgroup\$
    – Sᴀᴍ Onᴇᴌᴀ ♦
    Aug 29, 2017 at 16:01
  • \$\begingroup\$ I'm a bit critical of switching for loops to forEach(), since this is tagged performance. While it is a more idiomatic approach, it is definitely less performant. \$\endgroup\$
    – Patrick Roberts
    Sep 9, 2017 at 7:22
  • \$\begingroup\$ Patrick- that is what I was implying by the last sentence of the paragraph under Iterations. Thanks for your feedback. \$\endgroup\$
    – Sᴀᴍ Onᴇᴌᴀ ♦
    Sep 9, 2017 at 16:06
2
\$\begingroup\$

A couple of small points.

First, why is it an array of ints? Cells are dead or alive, so why not bools? You spend a lot of keystrokes turning integers into bools, and then turning those bools right back into integers. Just keep them bools!

Second, supposing that you do go with bools: this code can be made much simpler:

function deadOrAlive(x, y) {
  const surround = checkSurrounding(x, y);
  if (gameBoard[x][y] === 1 && (surround === 2 || surround === 3)) return 1;
  else if (gameBoard[x][y] === 0 && surround === 3) return 1;
  else return 0;
}

Could instead be

function deadOrAlive(x, y) {
  const surround = checkSurrounding(x, y);
  return surround === 3 || surround === 2 && gameBoard[x][y];
}

or, if you stick with ints, then

function deadOrAlive(x, y) {
  const surround = checkSurrounding(x, y);
  return surround === 3 || surround === 2 && gameBoard[x][y] === 1;
}

Note that in this code we now check the cheapest thing first, always. If surround is 3, then we're done without doing any more work. If surround is not 3 and not 2, then again, we're done without doing any more work. Only if surround is 2 do we do a double-array access.

Third: this inner-loop code spends a lot of code on the page, and a lot of execution time, figuring out how to loop:

function checkSurrounding(x, y) {
  let sum = 0;
  startX = x-1 < 0 ? x : x-1;
  stopX = x+1 >= canvasWidth ? x : x+1;
  startY = y-1 < 0 ? y : y-1;
  stopY = y+1 >= canvasHeight ? y : y+1;
  for (let i = startX ; i <= stopX; i++) {
    for (let j = startY ; j <= stopY; j++) {
      if (i !== x || j !== y) {
        sum += gameBoard[i][j];
      }
    }
  }
  return sum;
}

Another possible technique is to surround your board by a one-cell zone of death; these cells start dead and never become alive. If you simply change your loop to:

for (let i = 1; i < canvasWidth - 1; i++) {
  for (let j = 1; j < canvasHeight - 1; j++) {
    newBoard[i][j] = deadOrAlive(i, j);
  }
}

then you can change your inner loop to... well, not a loop!

function checkSurrounding(x, y) {
  return 
    (gameBoard[x-1][y-1] ? 1 : 0) + 
    (gameBoard[x-1][y] ? 1 : 0) + 
    (gameBoard[x-1][y+1] ? 1 : 0) + 
    (gameBoard[x][y-1] ? 1 : 0) + 
    (gameBoard[x][y+1] ? 1 : 0) + 
    (gameBoard[x+1][y-1] ? 1 : 0) + 
    (gameBoard[x+1][y] ? 1 : 0) + 
    (gameBoard[x+1][y+1] ? 1 : 0);
}

(Again, assuming that it is bools, not ints. If you keep it ints, then you just get to do the sum.)

Also, it should be countSurrounding, since that's what it does.

Improve this answer
\$\endgroup\$
2
  • \$\begingroup\$ Thanks Eric. Using bools is good idea. I will consider that for version 2. I have a question about function checkSurrounding(x, y) . What do you think about reducing the array instead of this long sum? \$\endgroup\$
    – korczas
    Aug 29, 2017 at 19:15
  • \$\begingroup\$ @Kamil: I think that if the task of the code is "add eight things together" then a sum with eight things in it is very appropriate. \$\endgroup\$
    – Eric Lippert
    Aug 29, 2017 at 20:01

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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