My minesweeper implementation in Javascript

Question

I'm beginner coder and I would like to request for critique review of my minesweeper implementation. Game was made using bootstrap 3 and game board itself is a simple html DOM table. Game is complete and working but I feel it could be written in more simple and elegant manner. What in my code can be improved/written in different/better way? In game page you can choose size of board, then you have to click "start" to generate board and start the game. Ctrl + click on cell to flag it and prevent accidental click.

Main function responsible for revealing cells:

function reveal(ev){
  var x = parseInt(this.getAttribute('x')),
      y = parseInt(this.getAttribute('y'));

  if(ev.ctrlKey == false){
    if(cells[getFieldId(x,y)].markedByPlayer == false){

      if(cells[getFieldId(x,y)].hasBomb == true){
        document.getElementById(x+'x'+y).innerHTML = '*';
        alert('Bomb! You have lost.');
        gameState = 'loss';
        revealMap();
      }else if(cells[getFieldId(x,y)].neighbourNumber > 0 && cells[getFieldId(x,y)].hasBomb != true){
        document.getElementById(x + 'x' +y).innerHTML = cells[getFieldId(x,y)].neighbourNumber;
        cells[getFieldId(x,y)].hasBeenDiscovered = true;
        pointColor(x,y,'midnightblue');
        document.getElementById(x+'x'+y).style.color = 'silver';
        revealFields(x,y);
      }else{
        revealFields(x,y);
      }

      if(checkVictoryCondition() == bombsNumber){
        alert('You have won!');
        revealMap();
      }
    }
  }else if(ev.ctrlKey == true){

    if(cells[getFieldId(x,y)].markedByPlayer == false){
      document.getElementById(x+'x'+y).innerHTML = '!';
      cells[getFieldId(x,y)].markedByPlayer = true;
      document.getElementById(x+'x'+y).style.color = 'red';
    }else if(cells[getFieldId(x,y)].markedByPlayer == true){
      document.getElementById(x+'x'+y).innerHTML = '';
      cells[getFieldId(x,y)].markedByPlayer = false;
    }
  }
}

function revealFields(x,y){
  if(x<0 || y<0 || x>boardHeight - 1 || y>boardWidth - 1){
    return;
  }
  if(cells[getFieldId(x,y)].neighbourNumber > 0){
    document.getElementById(x+ 'x' +y).innerHTML = cells[getFieldId(x,y)].neighbourNumber;
    cells[getFieldId(x,y)].hasBeenDiscovered = true;
    pointColor(x,y,'midnightblue');
    document.getElementById(x+ 'x' +y).removeEventListener('click', reveal, true);
  }
  if(cells[getFieldId(x,y)].hasBeenDiscovered == true){
    return;
  }
  cells[getFieldId(x,y)].hasBeenDiscovered = true;
  pointColor(x,y,'midnightblue');
  document.getElementById(x+ 'x' +y).removeEventListener('click', reveal, true);

  setTimeout(function(){revealFields(x-1,y);}, 200);
  setTimeout(function(){revealFields(x+1,y);}, 200);
  setTimeout(function(){revealFields(x,y-1);}, 200);
  setTimeout(function(){revealFields(x,y+1);}, 200);
  setTimeout(function(){revealFields(x-1,y-1);}, 200);
  setTimeout(function(){revealFields(x-1,y+1);}, 200);
  setTimeout(function(){revealFields(x+1,y-1);}, 200);
  setTimeout(function(){revealFields(x+1,y+1);}, 200);
}

JS fiddle link: https://jsfiddle.net/pL1n8zwj/1/

Answer 1

Here are my personal thoughts:

• Naming:

  • it's probably better to have placeBombs instead of initBombs, because you have a function called placeBomb.
  • Sometimes you use i and j , others you use x and y, it's better to be consistent.

• Structure:

  • You should have a state field with constants indicating the state of the cell instead of various fields set to true or false.
  • you don't really need to look for the value inside of an hash to get the value of a cell. You can have a two dimensional array and reference the cell directly. This also means that you can remove the getFieldId function.

Something like this, for example:

var states = {
    MARKEDBYPLAYER: 1,
    DISCOVERED: 2,
    UNTOUCHED: 3
};

var boardSize = 16;
var cells = new Array(boardSize);

for(var x=0; x < boardSize; x++) {
    cells[x] = new Array(boardSize);
    for(var y=0; y < boardSize; y++) {
        cells[x][y] = {};
        cells[x][y].state = states.UNTOUCHED;
        cells[x][y].hasBomb = true;
        cells[x][y].neighbourNumber = null;
  }
}

if (cells[0][1].hasBomb) {
    console.log('Player has lost')
}

• In generateBoard you have a bunch of instruction that would read better if you had just a call to a function that sets the required values.

Something like this:

function setDomCell(domCell, x, y, cellSize, cellFontSize) {
  domCell.id = x+'x'+y;
  domCell.style.width = cellSize;
  domCell.style.height = cellSize;
  domCell.style.fontSize = cellFontSize;
  domCell.setAttribute('x', x);
  domCell.setAttribute('y', y);
  domCell.addEventListener('click', reveal, true);
}

function generateBoard(){
  var domRow, domCell;

  for(var y=0; y<boardHeight; y++){
    domRow = document.createElement('tr');
    domBoard.appendChild(domRow);
    for(var x=0; x<boardWidth; x++){
      domCell = document.createElement('td');
      setDomCell(domCell, x, y, cellSize, cellFontSize);
      domRow.appendChild(domCell);
    }
  }
}

It would be even better if you had on object to take care of that, but take it one step at a time.

• In the start function you have a switch to lookup values based on another value.

I'd say that's what a hash is for:

  var sizeVars = {
    'small': {
      boardWidth: 10,
      boardHeight: 10,
      cellSize: '48px',
      cellFontSize: '32px',
      bombsNumber: 16
    },
    'medium': {
      boardWidth: 20,
      boardHeight: 20,
      cellSize: '32px',
      cellFontSize: '16px',
      bombsNumber: 70
    },
    'large': {
      boardWidth: 30,
      boardHeight: 30,
      cellSize: '16px',
      cellFontSize: '8px',
      bombsNumber: 160
    }
  }

  var chosenSize = document.getElementById('size').value;
  console.log(sizeVars[chosenSize].boardWidth);