Chomp game in javascript

Question

I tried to make a 2-player Chomp game in javascript:

const num_rows = 4;
const num_cols = 7;

var cur_player = 1;

var rows = [];
var buttons = [];

cur_player_text = document.createElement('div');
cur_player_text.innerText = "Current player: " + cur_player;
cur_player_text.id = "cur_player";
document.body.appendChild(cur_player_text);


for(var i = num_rows; i>0; i--) {
  rows[i] = document.createElement('div');
  rows[i].id = "row" + [i];
  document.body.appendChild(rows[i]);
  buttons[i] = [];

  for(var j = 1; j<=num_cols; j++) {
    buttons[i][j] = document.createElement('button');
    buttons[i][j].className = "chocolate";
    buttons[i][j].id = "button" + [i] + "," + [j];
    buttons[i][j].innerText = "x";
    buttons[i][j].setAttribute('onclick', 'squareClick(' + i + ', ' + j + ');');
    rows[i].appendChild(buttons[i][j]);
  }

}
buttons[1][1].setAttribute('disabled', '');

function squareClick(row, col) {
  var i = row;
  var j = col;

  while(buttons[i] != undefined && buttons[i][j] != undefined && buttons[i][j].getAttribute('disabled') == null) {
    while(buttons[i][j] != undefined && buttons[i][j].getAttribute('disabled') == null) {
      buttons[i][j].setAttribute('disabled', '');
      j++;
    }
    row = row+1;
    i = row;
    j = col;
  }

  if(buttons[1][2].getAttribute('disabled') == '' && buttons[2][1].getAttribute('disabled') == '') {
    document.getElementById('cur_player').innerText = "Winner: " + cur_player;
  }
  else {
    cur_player = cur_player%2 + 1;
    document.getElementById('cur_player').innerText = "Current player: " + cur_player;
  }

}

It seems to work but the code is very ugly. How can it be made cleaner and more elegant?

Answer 1

Here are my design and style suggestions:

• Format your code with Prettier and use a linter.
  • Be consistent with single or double quotes (Prettier will fix this).
  • if( should be if ( (Prettier will fix this).
  • Avoid lines longer than 70 chars or so (Prettier will fix this).
• Use const for all variables except for loop counters, use let (ESLint will fix this, along with most of the below).
• Prefer for over while loops.
• Never use == and !=. Always === and !== to avoid surprising type coercion bugs.
• Use camelCase, not snake_case for JS.
• Use kebab-case instead of snake_case for CSS.
• Don't dynamically generate ids. Use classes and indices or data attributes.
• Avoid storing and modifying state in the DOM. Keep your own data structures so you can ensure one-directional data flow to avoid bugs (reading/writing DOM properties comes with many gotchas and pushes the data into places outside of the module's private control).
• Instead of onclick, use addEventListener.
• Use row++; instead of row = row+1;.
• Use template literals rather than string concatenation for things like "button" + [i] + "," + [j];.
• Move complex conditions to functions.
• Break code into logical functions; consider a class, closure or module to collect the loose variables and functions into a unit.
• Avoid coding in the global scope.
• Use zero-indexing.

Suggested rewrite (I'm storing disabled state in the DOM, breaking my own rule--next refactor I'd move that into objects):

const Chomp = ({container, rows, cols}) => {
  // TODO throw if rows or cols are < 3 or container isn't an element

  const statusEl = document.createElement("div");
  const gridEl = document.createElement("div");
  container.append(statusEl, gridEl);
  const grid = [...Array(rows)].map(() => [...Array(cols)]);
  let currentPlayer;

  const isWon = () =>
    grid[rows-1][1].getAttribute("disabled") &&
    grid[rows-2][0].getAttribute("disabled");

  const updateStatus = () => {
    statusEl.innerText = isWon() ?
      `Winner: ${currentPlayer}` :
      `Current player: ${currentPlayer}`;
  };

  const move = (x, y) => {
    for (let yy = 0; yy <= y; yy++) {
      for (let xx = x; xx < grid[yy].length; xx++) {
        if (grid[yy][xx].getAttribute("disabled")) {
          break;
        }

        grid[yy][xx].setAttribute("disabled", true);
      }
    }

    currentPlayer = currentPlayer % 2 + 1;
  };

  const makeButton = (x, y) => {
    const btn = document.createElement("button");
    btn.textContent = "x";
    btn.addEventListener("click", () => {
      move(x, y);
      updateStatus();
    });
    grid[y][x] = btn;
    return btn;
  };

  const makeRow = y => {
    const row = document.createElement("div");

    for (let x = 0; x < cols; x++) {
      row.appendChild(makeButton(x, y));
    }

    return row;
  };

  const makeGrid = () => {
    for (let y = 0; y < rows; y++) {
      gridEl.appendChild(makeRow(y));
    }

    grid[rows-1][0].setAttribute("disabled", true);
  };

  return {
    play() {
      gridEl.innerHTML = "";
      currentPlayer = 1;
      makeGrid();
      updateStatus();
    }
  };
};

(() => {
  const container = document.querySelector(".chomp");
  const chomp = Chomp({rows: 4, cols: 7, container});
  chomp.play();
})();

<div class="chomp"></div>