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

• Furman: 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.