4
\$\begingroup\$

I would really like to get some feedback on how you would improve my code's style, readability, and efficiency. I failed an interview recently and was unable to get any feedback so I thought I would try asking here. I have cleaned up the code a bit already based on advice from friends but would like to have more critical eyes take a look.

My task was to write a function that takes a matrix of 1's and 0's and returns a resulting matrix after one iteration of Conway's Game of Life.

Note: Since the goal was to only return 1 iteration, I was told to not create cell objects and hold references to neighbors. There is not a need to set this up because we only want to run 1 iteration and doing so would waste space.

I would like advice on what you would change in terms of the function organization and how I decided to break down the problem. Specifically, it feels a little awkward how I have to pass the matrix all the way down to the isInBounds function. Is there a way to avoid this?

This or any other readability/optimization advice would be greatly appreciated.

function oneIterationConway(matrix) {
  const height = matrix.length;
  const width = matrix[0].length;
  const resultMatrix = [];

  for (let row = 0; row < height; row++) {
    resultMatrix[row] = [];
    for (let col = 0; col < width; col++) {
      const neighborCount = countNeighbors(row, col, matrix);
      let isAlive = 0;
      if (matrix[row][col]) {
        // current cell is alive
        isAlive = neighborCount > 1 && neighborCount < 4 ? 1 : 0;
      } else {
        // current cell is dead
        isAlive = neighborCount === 3 ? 1 : 0;
      }

      resultMatrix[row][col] = isAlive;
    }
  }

  return resultMatrix;
}

function countNeighbors(row, col, matrix) {
  let neighbors = 0;

  for (let i = -1; i <= 1; i++) {
    for (let j = -1; j <= 1; j++) {
      if (i === 0 && j === 0) continue;
      if (isInBounds(row + i, col + j, matrix) && matrix[row + i][col + j]) {
        neighbors++;
      }
    }
  }

  return neighbors;
}

function isInBounds(row, col, matrix) {
  const height = matrix.length;
  const width = matrix[0].length; 
  return row >= 0 && col >= 0 && row < height && col < width;
}


const testMatrix = [
  [1, 0, 0, 0, 0],
  [0, 1, 1, 0, 0],
  [0, 1, 0, 0, 0]
];

const result = oneIterationConway(testMatrix);

for (let row of result) {
  console.log(row);
}
\$\endgroup\$
  • \$\begingroup\$ Efficiency in JavaScript usually means obfuscation so readability and efficiency aren't usually the same thing. For readability, you could do something like isAlive = (neighborCount > 1 && neighborCount < 4) ? 1 : 0; or use an if/else, such as if( neighborCount > 1 && neighborCount < 4 ){isAlive = 1;}else{isAlive = 0;} \$\endgroup\$ – Shaun Bebbers Mar 23 '17 at 11:12
2
\$\begingroup\$

There are a couple of ways of solving this:

  1. Nest the other functions inside of oneIterationConway

    function oneIterationConway(matrix) {
      const height = matrix.length;
      const width = matrix[0].length;
      function isInBounds(row, col, matrix) {
        return row >= 0 && col >= 0 && row < height && col < width;
      }
    

    this way the first definition of height is in scope (available) inside isInBounds. Hiding your 'private' functions is a good idea anyway.

  2. Don't use isInBounds at all, take advantage of the fact that items out of bounds are undefined.

    function countNeighbors(row, col, matrix) {
      let neighbors = -matrix[row, col];
    
      for (let i = -1; i <= 1; i++) {
        for (let j = -1; j <= 1; j++) {
          let value = matrix[row+i, col+j];
          if (value) neighbors++;
        }
      }
      return neighbors;
    }
    
  3. If I am in control of the design, something I often do is add a boundary around my matrix. Something like:

    const testMatrix = [
      [0, 0, 0, 0, 0, 0, 0],
      [0, 1, 0, 0, 0, 0, 0],
      [0, 0, 1, 1, 0, 0, 0],
      [0, 0, 1, 0, 0, 0, 0],
      [0, 0, 0, 0, 0, 0, 0],
    ];
    

My loops would then be from 1 to size-2

The only other obvious thing I would be would be to factor out part of the main loop:

for (let row = 0; row < height; row++) {
  resultMatrix[row] = [];
  for (let col = 0; col < width; col++) {
    resultMatrix[row][col] = isAlive(row, col);
  }
} 

function isAlive(row, col) {
  const neighborCount = countNeighbors(row, col);
  if (matrix[row][col]) {
    // current cell is alive
    return neighborCount > 1 && neighborCount < 4 ? 1 : 0;
  } else {
    // current cell is dead
    return neighborCount === 3 ? 1 : 0;
  }
}

btw, There is a nice feature in most environments. You can use: console.table(results)

\$\endgroup\$
  • \$\begingroup\$ I originally had my functions nested inside of oneIterationConway but I decided to move it out because I read that javascript will initialize these functions each time the parent is called. If you knew this was going to be run many times, is there a way to have the height and width outside of the inner functions scope without defining this as a function? Would it make sense to create an object which holds all of the functions and properties? \$\endgroup\$ – noobycoder Mar 23 '17 at 21:44
  • \$\begingroup\$ You are correct. It would create new function instances each time. There are several ways to deal with this in JS the most common being either defining those methods on the prototype or using ES6 classes. You can also export an interface something like the first answer here stackoverflow.com/questions/11525782/data-hiding-in-javascript \$\endgroup\$ – Marc Rohloff Mar 24 '17 at 1:56
1
\$\begingroup\$

I find it strange to use 1 and 0 for boolean values true and false. Instead of this:

isAlive = neighborCount > 1 && neighborCount < 4 ? 1 : 0;

isAlive = neighborCount === 3 ? 1 : 0;

It would be more natural to write:

isAlive = neighborCount > 1 && neighborCount < 4;

isAlive = neighborCount === 3;

That is, I simply dropped the ternaries.

It's even better to wrap the check in a function as @MarcRohloff already pointed out.

\$\endgroup\$

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.