9
\$\begingroup\$

I created my first JavaScript program and I feel it's all over the place. I just started it without thinking too much about it. Now, I am quite sure I should have used OO style.

Also, it seems there is a lot of repetitive code.

Anyway, as this is my first JS project I would be thankful for all feedback or any pointer to a topic I should look into.

//surpresses rightclick menu
window.oncontextmenu = (e) => {
    e.preventDefault();
}

var field = {
    cols: 0,
    rows: 0,
    mines: 10,
    markedMines: 0,
    bombArray: [],
    gameover: false
    

    //checks if every field with a mine is  flagged. if not, returns false
    , checkWin: function () {
        let check = true;
        for (let i = 1; i <= field.cols; i++) {
            for (let j = 1; j <= field.rows; j++) {
                let newId = parseInt(i);
                newId += "/";
                newId += parseInt(j);
                if (field.bombArray[i][j] == "x" && document.getElementById(newId).style.backgroundColor != "red") check =
                    false;
            }
        }
        return check;
    }

    //injects the gamefield into html- td id = i/j - left click reveals a field, right click flags a field
    , printTable: function (cols, rows) {

        let html = '';
        let id = '';
        html += '<table class="field">';
        for (i = 1; i <= cols; i++) {
            html += '<tr>';
            for (j = 1; j <= rows; j++) {

                html += '<td id="' + i + '/' + j + '" onclick=field.reveal(id) oncontextmenu=field.markABomb(id) align="center">';
                html += '</td>';
            }
            html += '</tr>';
        }
        html += '</table>';
        return html;
    }
    
    // functioncall with rightclick on <td>
    , markABomb: function (id) {
        let x = id.substring(0, id.indexOf("/"));
        let y = id.substring((id.indexOf("/") + 1), (id.length + 1));

        if (!this.gameover) {

            if (document.getElementById(id).style.backgroundColor != "yellow") {
                if (document.getElementById(id).style.backgroundColor == "red") {
                    document.getElementById(id).style.backgroundColor = "blue";
                    field.markedMines--;
                    field.mines++;
                } else {
                    document.getElementById(id).style.backgroundColor = "red";
                    field.markedMines++;
                    field.mines--;
                }
            } else {

                //if flagged mines on adjacant fields are equal to number on the field, reveal all adjacent fieds. put maybe into own funtion?
                if (this.bombArray[x][y] === this.countRedNeighbours(x, y)) {
                    for (let i = -1; i < 2; i++) {
                        for (let j = -1; j < 2; j++) {
                            let id = parseInt(parseInt(x) + parseInt(i));
                            id += "/";
                            id += parseInt(parseInt(y) + parseInt(j));
                            this.reveal(id);
                        }
                    }
                }
            }
            this.updateShowCounter();

            if (this.mines == 0) {
                if (this.checkWin()) {
                    alert("congratulations. You won!");
                    this.gameover = true;
                } else alert("somethings not quite right");
            }
        }
    }


    , updateShowCounter: function () {
        document.getElementById("marked").innerHTML = ("<p color:white>marked: " + this.markedMines + "</p>");
        document.getElementById("minesLayed").innerHTML = ("<p color:white>mines: " + this.mines + "</p>");
    }

//counts adjacent flagged mines. what a mess. 
    , countRedNeighbours: function (x, y) {
        let counter = 0;
        for (let i = -1; i < 2; i++) {
            for (let j = -1; j < 2; j++) {
                let id = "";
                id = parseInt(parseInt(x) + parseInt(i));
                let ix = parseInt(parseInt(x) + parseInt(i));
                id += "/";
                id += parseInt(parseInt(y) + parseInt(j));
                let yj = parseInt(parseInt(y) + parseInt(j));
                let z = field.bombArray.length - 1;
                if (ix > 0 && yj > 0 && ix < field.bombArray.length - 1 && yj < this.bombArray[z].length - 1) {
                    if (document.getElementById(id).style.backgroundColor == "red") counter++;
                }
            }
        }
        return counter;
    }

    , revealAllBombs: function (cols, rows) {
        for (let i = 0; i <= parseInt(cols); i++) {
            for (let j = 0; j <= (rows); j++) {
                if (field.bombArray[i][j] == "x") {
                    if (i < this.bombArray.length - 1 && j < this.bombArray[this.cols].length - 1) {
                        let id = parseInt(i);
                        id += "/";
                        id += parseInt(j);
                        document.getElementById(id).innerHTML = "X";
                        document.getElementById(id).style.backgroundColor = "red";
                    }
                }
            }
        }
    }
    //functioncall from <td>, left-click
    , reveal: function (id) {
        if (!this.gameover) {
            let x = id.substring(0, id.indexOf("/"));
            let y = id.substring((id.indexOf("/") + 1), (id.length + 1));
            if (x > 0 && y > 0 && x < this.bombArray.length - 1 && y < this.bombArray[this.cols].length - 1) {

                if (document.getElementById(id).style.backgroundColor != "red") {


                    if (field.bombArray[x][y] == "x") {
                        field.revealAllBombs(this.cols, this.rows);
                        alert("lost");
                        this.gameover = true;
                    } else if (field.bombArray[x][y] == "0") {
                        field.revealAll0(id);
                    } else {
                        document.getElementById(id).style.backgroundColor = "yellow";
                        document.getElementById(id).innerText = field.bombArray[x][y];
                    }
                }
            }
        }
    }

    , buildArray: function (cols, rows) {
        cols++;
        rows++;
        for (let i = 0; i <= parseInt(cols); i++) {
            field.bombArray[i] = [];
            for (let j = 0; j <= parseInt(rows); j++) {
                field.bombArray[i][j] = "";
            }
        }
    }

    , setMine: function (cols, rows) {
        let x = Math.floor((Math.random() * cols));
        let y = Math.floor((Math.random() * rows));
        x++;
        y++;
        field.bombArray[x][y] = "x";
    }

    , countMines: function (cols, rows) {
        let mineCounter = 0;
        for (let i = 1; i <= cols; i++) {
            for (let j = 1; j <= rows; j++) {
                if (field.bombArray[i][j] == "x") mineCounter++;
            }
        }
        return mineCounter;
    }

    //counts all adajacent X and writes it in bpmbArray
    , countNeighbours: function (cols, rows) {

        for (let x = 1; x < cols + 1; x++) {
            for (let y = 1; y < rows + 1; y++) {
                let counter = 0;
                for (let i = -1; i < 2; i++) {
                    for (let j = -1; j < 2; j++) {
                        let xi = x + i;
                        let yj = y + j;

                        if (xi > 0 && yj > 0 && xi < this.bombArray.length && yj < this.bombArray[this.cols].length) if (this.bombArray[parseInt(x + i)][parseInt(y + j)] === "x") counter++;
                    }
                }
                if (x > 0 && y > 0 && x < this.bombArray.length && y < this.bombArray[this.cols].length) if (field.bombArray[x][y] != "x") field.bombArray[x][y] = parseInt(counter);
            }
        }
    }

    // if a field with 0 mines next to it is revealed, all fields with 0 are revealed
    , revealAll0: function (id) {

        let listXY = [];
        listXY.push([parseInt(id.substring(0, id.indexOf("/"))), parseInt(id.substring((id.indexOf("/") + 1), (id.length + 1)))]);

        while (listXY.length > 0) {
            let x = parseInt(listXY[0][0]);
            let y = parseInt(listXY[0][1]);
            for (let i = -1; i < 2; i++) {
                for (let j = -1; j < 2; j++) {
                    if ((x + i != 0 && y + j != 0 && (x + i) <= document.getElementById("cols").value && (y + j) <= document
                        .getElementById("rows").value)) {
                        let newId = parseInt(x + i);
                        newId += "/";
                        newId += parseInt(y + j);
                        if (document.getElementById(newId).style.backgroundColor == "red") {
                            field.mines++;
                            field.markedMines--;
                            this.updateShowCounter();
                        }
                        if (field.bombArray[x + i][y + j] != "x" && document.getElementById(newId).style.backgroundColor !=
                            "yellow") {

                            document.getElementById(newId).style.backgroundColor = "yellow";
                            if (field.bombArray[x + i][y + j] != 0) document.getElementById(newId).innerText = field.bombArray[
                                x + i][y + j];
                            if (field.bombArray[x + i][y + j] == 0) {
                                listXY.push([x + i, y + j])
                            }
                        }
                    }
                }
            }
            listXY.shift();
        }
    }
}

//-----------------------------------Object ends here---------------------------------------------------------------------------

function insertHTML(id, html) {
    let el = document.getElementById(id);
    el.innerHTML = html;
}


run = () => {
    field.gameover = false;
    let cols = document.getElementById("cols").value;
    field.cols = cols;
    let rows = document.getElementById("rows").value;
    field.rows = rows;
    field.mines = document.getElementById("mines").value;
    field.markedMines = 0;
    field.updateShowCounter();


    if ((field.cols * field.rows < field.mines) || cols <= 1 || rows <= 1 || cols > 100 || rows > 100 || field.mines < 1) {
        alert("so nicht Kevin")
        field.gameover=true;
    } else {

        let html = field.printTable(cols, rows);
        field.buildArray(cols, rows);
        while (field.countMines(cols, rows) < field.mines) field.setMine(cols, rows);
        field.countNeighbours(cols, rows);
        insertHTML('table', html);
    }
}

// Run everything when the document loads.
window.onload = run;
html {
    font-family: sans-serif;
  }

  .wrapper {
    display: grid;
    grid-template-columns: 200px, 8fr;
    grid-template-areas: 
    "....... header"
    "settings content";
    justify-content: left;
    grid-gap: 20px;
    white-space:nowrap;

  }
  .header {
      align-items: end;
      height: 10px;
      grid-area: header;
      color: white;
      margin-top: 10px;
  }
  .settings{
    grid-area: settings;
    position: absolut;
    white-space: nowrap;
    margin-top: 50px;
  }
  input {
    -moz-appearance: textfield;
    text-align: right;
    text-decoration: none;
    border: 0;
    background: black;
    border-bottom: 1px solid;
    color: #ffffff;
    width: 75px;
}

input[type=number]::-webkit-inner-spin-button,
input[type=number]::-webkit-outer-spin-button {
    opacity: 0;
}

  .content {
    grid-column-start: 2;
    grid-area: content;
    margin-top: 50px;
  }

  body {
    background: black;
  }
  #manual{
      color: white;
      font-size: small;
      opacity: 20%;
  }
  #manual:hover{
      opacity: 75%;
  }
 
  td {
    background: blue;
    height: 20px;
    width: 20px;

  }
  td:hover{
      background: rgba(0, 0, 255, 0.527);
  }
  h1{
      text-align: right;
  }

  p {
    color: white;
  }

  button {
    margin-top: 20px;

  }

  .marked {
    color: red;
    margin-top: 25px;
  }

  .minesLayed {
    color: red;
    margin-top: 25px;
  }
  
<!DOCTYPE html>
<html>
  <head>
    <title>Minesweeper</title>

    <link rel="stylesheet" type="text/css" href="style.css" />

    <script
      language="javascript"
      type="text/javascript"
      src="script.js"
    ></script>
  </head>

  <body>
    <div class="wrapper">
      <div class="header"><h1>.Minesweeper</h1></div>
      <div id="settings" class="settings" align="right">
        <p>
          columns:
          <input type="number" name="rows" id="rows" value="16" />
        </p>

        <p>
          rows:
          <input type="number" name="cols" id="cols" value="16" />
        </p>
        <p>mines: <input type="number" name="mines" id="mines" value="40" /></p>
        <button type="submit" id="submit" lang="Name" onclick="run()">
          Start
        </button>

        <div class="minesLayed" id="minesLayed"></div>

        <div class="marked" id="marked"></div>

      </div>

      <div id="table" class="content"></div>
    </div>
    <p id="manual">Your goal is to flag all mines. Left-click a blue square to reveal it.<br>
        Right-click a blue square to flag it. A flaged quare is red.
        Right-click a red quare to unflag it.<br>
      A number represants the adjacent mines. If all adjacent mines are flaged, right-click it to reveal its adjacant squares</p>

  </body>
</html>

\$\endgroup\$

2 Answers 2

4
\$\begingroup\$

Code Review

Great work! Programming is a skill that most find near impossible to pick up. Your game clearly demonstrates you have the mind of a coder and just getting an app to work, as a beginner, deserves full points.

I could only see one bug and that is the page will lock up if the number of mines is greater than the number of cells.

Your code does show your lack of experience (which is not a bad thing we all started this way) so this review is rather long as there are many items to cover. The example rewrite may also be a little advanced so if you have questions please do ask in the comments and I will reply in comment or update this answer to clarify any points.

This answer should contain many links but as there are so many I have just included this MDN JavaScript reference From that page you can find all you will need regarding and of the functions and objects mentioned in this answer.

Logic, layout and design.

Use the DOM

You create a table with columns and rows however you reference (by name) the columns as row and the rows as columns. This type of missing naming can create serious problems when maintaining the code. Always be precise in your naming and abstraction.

Single event

You add events for every cell in the game. You are far better of adding one event listener to the tables and in that listener using the event object to get the cell that triggered the event. see example field.cellClicked

String.split

Extracting content from a string. You get the row and column of a cell using the Id of an element. First the id should never represent any type of information, it is just an identifier. Second your method of extraction is somewhat clunky

You have

let x = id.substring(0, id.indexOf("/"));
let y = id.substring((id.indexOf("/") + 1), (id.length + 1));

you could use

let [x, y] = id.split("/");
// or
const [x, y] = id.split("/").map(Number);

Keep out of global scope

Keep the global scope clean. Put the whole game inside a function (see example where the game is contained within the load event). This makes the code far more portable and safe to insert into other pages. Pages that will contain 3rd Party code.

Complexity is the enemy

Your code is in many parts very repetitive and complex (mostly due to inexperience). You often repeat the same logic to find the same state in various parts of the code.

Your data structure is also too complex, structuring it to match the game layout rather than what you need to manage the game state.

Always aim to keep it as simple as possible. Don't store information not needed, don't compute information more than once.

Warning

Always in Strict mode

You have some undeclared variables in use (variable without var, let, or const). This is very dangerous as undeclared variables are created in global scope and can easily be used in various places without you noticing that they are all the same reference.

To prevent these types of easy to make oversights always run your scripts in strict mode. To set strict mode just add the line "use strict"; at the top of the JavaScript file.

Query the DOM

DOM queries are very slow compared to using a stored or direct reference to the elements.

document.getElementById(newId) is a DOM query and you use it all over the place. As this is not a realtime game its not that big of a deal, but it is a bad habit and should be avoided.

Locate and store a reference to all the DOM elements you need once at the start of the code and use those references when you need to do something to an element. see example field.elements object that is used to store elements needed in the game

User friendly

You might create the best game ever but with a clunky and hard to use UI nobody will evry want to play it.

Try to not give users options that are invalid. Eg you provide inputs to set the rows and columns, in the function run you then check if the values are <= 1 or > 100 and display an alert. The user should never be able to add these invalid settings. This is a basic UI design rule you should always consider.

Provide as much feedback as you can to let the user know what can and can not be done. Eg I have added mouse pointer only to cells that can be clicked. You can add more and custom pointers to.

DO NOT use alerts. Often they can be deactivated so you can never know if the user has seen them. They are also blocking and if there are problems (repeated calls to alert) you can leave such a bad taste in the user that they will never come back.

CSS

Use CSS style rules to define styles and use these styles to modify element styles. See Example. You can access the elements class name via element.className and also element.classList for a higher level interface.

General style points

-Use object function shorthand to reduce code noise. eg checkWin: function () { becomes checkWin() {

  • Use function declarations eg function run( rather than function expressions const run = () => { as function declarations can be call from any point in the code while function expressions can only be called after the expression has been executed.

  • Use addEventListener to add event rather than assigning to the onEvent property of the element.

  • Use const for variables that do not change.
  • Why are you constantly using parseInt (OMDG parseInt(parseInt(x) + parseInt(i)) !!!) You only need to do this if the variable is a string representation of the number and often it is better to use Number("1") to convert to a number than parseInt.

  • `Don't create content using HTML. Use the DOM interface to create elements to add to the pages. It is 100s of times quicker and prevents a whole pile of problems with the page settings.

  • Use strict equality and inequality === and !== rather than truthy equalities == and !=.

  • Avoid single use variables unless they help in readability (eg make a line shorter).

    Example of poor use of variables let cols = document.getElementById("cols").value; only used once on the next line. Much better as field.cols = document.getElementById("cols").value;

  • DO NOT add JavaScript to the HTML. eg onclick="run()" should be set via addEventListener in the JavaScript code.

Naming

Your code is full of very bad naming practices.

  • You use cols, and rows to define row and column counts. Generally we use rows and cols to reference collections or arrays. Use colCount and rowCount to clearly define what the variables represent.
  • You call mines mine and bomb, this makes it very difficult to know what the variable name should be. In the example I have changed the names of all bomb references to mine.
  • Do not include the variables type in the name eg field.bombArray should be field.bombs the plural hits that the variable is an array or array like.
  • You are mixing field and this within the field object. As field is a single instance use field rather than this this is used when you have many instances of an object.

OK that will do it. I have not covered everything by there is a size limit to answers here.

Rewrite example

I have completely rewritten the code from the ground up (well almost). Have a look and take what you can from it.

Some points

  • Mines and Cells are stored in one dimensional array. X and y coords are converted to index to locate neighbors.
  • A cell object created by createCell contains all the logic and states of a cell
  • Recursive reveal The cell reveal function calls reveal on its neighbors if needed. This automatically and efficiently reveals all the neighbors without a lot of complicated repeated searches.
  • CSS rules to reflex cell state eg class="gameCell hidden mine marked"
  • One event listener handles left and right clicks. Listeners are removed when not needed.
  • field.reset starts the game rather than a function independent of field
  • Some helper function, game constants are stored at the beginning of the code.
  • "Use strict" at the top to ensure there are no simple typos and forgot it's in the code
  • Used getters and setters on the cell object
  • Used short circuit style to avoid to much code noise. Eg x < 0 && neighbours.push( is the same as if (x < 0) { neighbours.push(
  • Used ternaries to reduce code noise. eg text = mine ? "X" : (mineCount > 0 ? mineCount : ""); is the same as if (mine) { text = "X" } else { if (minecount > 0) { text = mineCount } else { text = "" } }`

"use strict";  
addEventListener("load", () => {  // isolates the game from the global scope

    // utility functions
    const createElement = (tag, props ={}) => Object.assign(document.createElement(tag), props);
    const byId = id => document.getElementById(id);
    
    // constants
    const MINE_CHAR = "\ud83d\udca3";
    const EXP_CHAR = "\ud83d\udca5";
    const LOSS = "You loss!";
    const WIN = "You win!";
    const PLAY_AGAIN = "Click start?";
    const DEBUG = false; // set to true to help debug game
    
    // The 0 timeout just waits till all the code below has been executed 
    // so that the setup code can hang out at the top 
    setTimeout(() => {            
            newGame.addEventListener("click",field.reset);
            field.reset();
        },0);
    
    function createCell(x, y) {
        var mine = false;
        var hidden = true;
        var marked = false;
        const neighbours = [];
        var element, mineCount;
        const API = {
            set element(el) { element = el }, 
            get isMine() { return mine },
            get isHidden() { return hidden },
            get isMarked() { return marked },
            updateNeighbours(field) {
                x > 0 && neighbours.push(field.cells[x - 1 + y * field.colCount]);
                y > 0 && neighbours.push(field.cells[x + (y - 1) * field.colCount]);
                x < field.colCount - 1 && neighbours.push(field.cells[x + 1 + y * field.colCount]);
                y < field.rowCount - 1 && neighbours.push(field.cells[x + (y + 1) * field.colCount]);
                mineCount = neighbours.reduce((count, cell) => count + (cell.isMine ? 1 : 0), 0);
                if (DEBUG) {
                    element.textContent = mine ? MINE_CHAR  : (mineCount ? mineCount : " " );
                }
            },
            makeMine() {
                if (!mine) {
                    mine = true;
                    element.classList.add("mine");
                    return API;
                }
            },
            show(exploded = false) {
                hidden = false;
                element.classList.remove("hidden");
                if (mine) {
                   if (exploded) {
                       element.textContent = EXP_CHAR; 
                       element.classList.remove("safe");
                   } else {
                       element.textContent = MINE_CHAR;
                       element.classList.add("safe");
                   }
                } else {
                   element.textContent = mineCount > 0 ? mineCount : "";
                }
            },          
            toggleMark(field) {
                marked = !marked;
                if (marked) {
                    field.marked ++;
                    element.classList.add("marked");
                } else {
                    field.marked --;
                    element.classList.remove("marked");
                }
            },
            reveal() {
                this.show();
                if (!mine && mineCount === 0) {
                    neighbours.forEach(cell => cell.isHidden && cell.reveal()); 
                }
            }
        };
        return API;
    }

    const field = {
        elements: {
            marked: byId("marked"),
            minesLayed: byId("minesLayed"),
            colCount: byId("colCount"),
            rowCount: byId("rowCount"),
            mineCount: byId("mineCount"),
            gameField: byId("gameField"),
            newGameBtn: newGame,
        },
        colCount: 0,
        rowCount: 0,
        maxMines: 10,
        marked: 0,
        mines: [],
        cells: [],
        reset() {
            field.elements.newGameBtn.textContent = "Restart";
            field.colCount = Number(field.elements.colCount.value);
            field.rowCount = Number(field.elements.rowCount.value);
            field.maxMines = Number(field.elements.mineCount.value);
            field.mines.length = 0;
            field.cells.length = 0;
            field.marked = 0;
            field.createBoard();
            field.updateDisplay();
        },
        endGame(message){
            field.elements.marked.textContent = message;   
            field.elements.minesLayed.textContent = PLAY_AGAIN;
            field.elements.table.removeEventListener("click",field.cellClicked);
            field.elements.table.removeEventListener("contextmenu",field.cellClicked);  
            field.elements.newGameBtn.textContent = "Start";                
        },
        checkWin() { 
            if (field.mines.every(mine => mine.isMarked)) {
                field.showAllCells();
                field.endGame(WIN);
            }
        },
        createBoard() {
            var x,y;
            field.elements.gameField.innerHTML = "";
            const table = field.elements.table = createElement("table",{className: "field"});
            for (y = 0; y < field.rowCount; y++) {
                const row = table.insertRow()
                for (x = 0; x < field.colCount; x++) {
                    const cell = createCell(x,y);
                    field.cells.push(cell);
                    cell.element = Object.assign(
                        row.insertCell(), {
                            className: "gameCell hidden", 
                            game_cell: cell,  // Use snake_case when assigning props
                        }
                    );
                }
            }
            while (field.mines.length < field.maxMines && field.mines.length < field.cells.length) { 
                const mine = field.cells[Math.random() * field.cells.length | 0].makeMine();  
                if (mine) { field.mines.push(mine) }
            }
            field.cells.forEach(cell => cell.updateNeighbours(field));
            field.elements.gameField.appendChild(table);
            table.addEventListener("click",field.cellClicked);
            table.addEventListener("contextmenu",field.cellClicked);    
        },
        updateDisplay() {
            field.elements.marked.textContent = field.marked;
            field.elements.minesLayed.textContent = field.mines.length - this.marked;
        },
        showAllMines(exp = false) { field.mines.forEach(mine => mine.show(exp)) },
        showAllCells() { field.cells.forEach(cell => cell.show()) },
        cellClicked(event) {
            if (event.target.game_cell) {
                if (event.type === "contextmenu") {
                    event.preventDefault();
                    event.target.game_cell.toggleMark(field);
                    field.updateDisplay();
                    field.checkWin();               
                } else {
                    event.target.game_cell.reveal();
                    if (event.target.game_cell.isMine) {
                        field.showAllMines();
                        setTimeout(() => {
                                field.showAllMines(true);                            
                                field.endGame(LOSS);
                            }, 1000
                        );
                    }
                }
            }
        },
    };

});
html {
    font-family: sans-serif;
 }

 .wrapper {
    display: grid;
    grid-template-columns: 200px, 8fr;
    grid-template-areas:
        "settings content";
    justify-content: left;
    grid-gap: 20px;
    white-space:nowrap;

 }

 .settings{
    grid-area: settings;
    position: absolut;
    white-space: nowrap;
    margin-top: 2px;
 }
 input {
    -moz-appearance: textfield;
    text-align: right;
    text-decoration: none;
    border: 0;
    background: black;
    border-bottom: 1px solid;
    color: #ffffff;
    width: 75px;
}

input[type=number]::-webkit-inner-spin-button,
input[type=number]::-webkit-outer-spin-button {
    opacity: 0;
}

 .content {
    grid-column-start: 2;
    grid-area: content;
    margin-top: 2px;
 }

 body {
    background: black;
 }

 
.gameCell {
    height: 24px;
    width: 24px;
    
    text-align: center;
    background: yellow; 
 }

.gameCell.hidden {
	 background: blue; 
     cursor: pointer;
}
.gameCell.hidden:hover{
    background: rgba(0, 0, 255, 0.527);
}
.gameCell.mine {
	 background: red; 
}
.gameCell.mine.safe {
	 background: #0D0; 
}
.gameCell.hidden.mine {
	 background: blue; 
}
.gameCell.hidden.mine.marked {
	background: red; 
    cursor: pointer;
}
.gameCell.hidden.marked {
	background: red; 
    cursor: pointer;
}



 p {
    color: white;
 }

 button {
    margin-top: 20px;

 }

 .markedMines {
    color: white;
    margin-top: 25px;
 }

 .minesLayed {
    color: white;
    margin-top: 25px;
 }
<div class="wrapper">

        <div id="settings" class="settings" align="right">
			<p>columns:<input type="number" id="rowCount" value="5" min="2" max="100"/></p>
			<p>rows:<input type="number" id="colCount" value="5" min="2" max="100"/></p>
			<p>mines: <input type="number" id="mineCount" value="5" /></p>
			<button id="newGame">Start</button>
			<div class="minesLayed" id="minesLayed"></div>
			<div class="markedMines" id="marked"></div>
		</div>
		<div id="gameField" class="content"></div>
    </div>

Update

I have just added some cosmetic changes. Removed the header so that the small game board fits the CodeReview snippet a little better.

\$\endgroup\$
6
  • \$\begingroup\$ thank you so much for the effort, thats exactly what i needed! This is a lot of information, and it will take time to go through this and come up with follow up questions! \$\endgroup\$
    – dan-el
    Commented Nov 18, 2019 at 13:06
  • \$\begingroup\$ Although this is an excellent answer, it's still somewhat opinionated. Not everyone codes this way, and I would certainly advise not to use object literals as classes, and then wrap those in a function, in another function. It's still a bit hard to parse. \$\endgroup\$
    – Kokodoko
    Commented Nov 21, 2019 at 13:40
  • \$\begingroup\$ @Kokodoko JavaScript only has Objects, classes do not exist. All the major browsers cache functions and use references to the cached functions so there are ZERO memory and performance hits using functions within functions as opposed to defining a prototype or (the flawed) class syntax. This is code review and as such (like your comment) appraisals are subjective and thus unavoidably opinionated. It is my opinion that encapsulation is fundamental in OO design, and in JavaScript this answer's style is the most effective way (using closure) of providing robust encapsulation. No # prefixing \$\endgroup\$
    – Blindman67
    Commented Nov 21, 2019 at 14:45
  • \$\begingroup\$ @SaulMarquez ES6 uses the "class" syntax to define objects. There is no type in javascript called "class". typeof something === "class" will NEVER be true. References created from the class syntax are always Objects \$\endgroup\$
    – Blindman67
    Commented Nov 21, 2019 at 17:16
  • \$\begingroup\$ Yes, that is true, ES6 classes are not true classes. But for basic use cases they behave like classes and this is OP's first JS project. I think the useful white lie is better than the confusing truth in this case. \$\endgroup\$
    – user50167
    Commented Dec 5, 2019 at 22:33
3
\$\begingroup\$

Definitely don't do this. The value should be on the same line as the variable being set. People read lines of code line-by-line, it's really confusing to see a lonely false;. Also, use line braces { } even when the code to be executed is 1 line long. And put the code all on a new line)

if (field.bombArray[i][j] == "x" && document.getElementById(newId).style.backgroundColor != "red") check =
                false;

Try to keep code separated in ways that make sense. Marking a bomb should not contain logic for checking if the player won. You also shouldn't run the checkWin method here.

For game-related reasons, checking if player won when a bomb is marked doesn't make sense. Does the player need to mark all bombs to win?

Stay away from magic numbers / magic Strings, use constants instead.

Use comments to explain parts of your code. What you are doing with the newId variable could probably be done better/differently, but tbh It's hard to follow. You could make it into a method (And still document if the name doens't explain well enough).

\$\endgroup\$
1
  • \$\begingroup\$ ah yes, that line break was made by one of the beautifier i tried. did not catch them all to undo them. the checkWin function is only called if the amount of flagged fields is equal to the amount of mines. \$\endgroup\$
    – dan-el
    Commented Nov 15, 2019 at 14:27

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.