7
\$\begingroup\$

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/

\$\endgroup\$
4
\$\begingroup\$

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);
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thank you for your answer. I'll try to change coding style according to your advices. However to clarify one thing: I always use i and j names for iterable variables inside for loop and x, y for variables describing coordinates. Don't know if it's bad coding habit but it seems to be rational to me. \$\endgroup\$ – Furman Jul 22 '16 at 6:34

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.