HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavascriptMinor

My minesweeper implementation in Javascript

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
javascriptimplementationminesweeper

Problem

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(xboardHeight -

Solution

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);

Code Snippets

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')
}
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);
    }
  }
}
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);

Context

StackExchange Code Review Q#135489, answer score: 4

Revisions (0)

No revisions yet.