3
\$\begingroup\$

It's been a while since I did anything with JavaScript. I made a tile puzzle game to help me brush up. You can find it here: https://compucademy.net/tile-puzzle/

I would like some feedback on my use of JS, HTML and CSS. Anything would be helpful - structure, best practices, overall approach, suggestions for improvement etc.

Any input much appreciated.

document.addEventListener( 'DOMContentLoaded', () => {
    const n = 16;
    const initialTileArray = [];
    for ( let i = 0; i < n; i++ ) {
    initialTileArray.push( String( i + 1 ) );
    }
    const board = document.querySelector( '.game-board' );
    const audio = document.getElementById( 'audio' );
    const resetButton = document.getElementById( 'reset-btn' );
    const scrambleButton = document.getElementById( 'scramble-btn' );
    let selectedTiles = [];
    let clickNumber = 0;
    let scrambledTileValues = [];
    resetButton.addEventListener( 'click', resetGame );
    scrambleButton.addEventListener( 'click', scrambleTiles );

    function resetGame() {
    board.innerHTML = '';
    scrambledTileValues = initialTileArray.slice( 0 );
    createBoard();
    }

    function scrambleTiles() {
    board.innerHTML = '';
    scrambledTileValues.sort( () => 0.5 - Math.random() );
    createBoard();
    }

    function createBoard() {
    for ( let i = 0; i < scrambledTileValues.length; i++ ) {
        const tile = document.createElement( 'div' );
        tile.setAttribute( 'data-pos', i + 1 );
        tile.classList.add( 'game-tile', 'game-blue', 'text-center' );
        tile.addEventListener( 'click', processTile );
        board.appendChild( tile );
        tile.innerHTML = scrambledTileValues[i];
    }
    }

    function checkWin() {
    const tiles = board.childNodes;
    const config = [];
    for ( var i = 0; i < tiles.length; i++ ) {
        config.push( tiles[i].innerHTML );
    }
    return JSON.stringify( config ) === JSON.stringify( initialTileArray );
    }

    function playSuccessSound() {
    audio.play();
    }

    function processTile() {
    const tilePos = this.getAttribute( 'data-pas' );
    if ( clickNumber === 0 || clickNumber === 1 ) {
        this.classList.remove( 'game-blue' );
        this.classList.add( 'game-red' );
        selectedTiles.push( this );
        clickNumber += 1;
    } else if ( clickNumber === 2 ) {
        selectedTiles.push( this );
        for ( var i = 0; i < selectedTiles.length; i++ ) {
        selectedTiles[i].classList.remove( 'game-red' );
        selectedTiles[i].classList.add( 'game-blue' );
        }
        window.console.log( this.getAttribute( 'data-pos' ) );
        const val0 = selectedTiles[0].innerHTML;
        const val1 = selectedTiles[1].innerHTML;
        const val2 = selectedTiles[2].innerHTML;
        selectedTiles[0].innerHTML = val2;
        selectedTiles[1].innerHTML = val0;
        selectedTiles[2].innerHTML = val1;
        selectedTiles = [];
        clickNumber = 0;
        if ( checkWin() ) {
        playSuccessSound();
        }
    }
    }
    resetGame();
} );



<!doctype html>
<html lang="en">
    <head>
        <!-- Required meta tags -->
        <meta charset="utf-8">
        <meta name="viewport" content="width=device-width, initial-scale=1, shrink-to-fit=no">
        <!-- Bootstrap CSS -->
        <link rel="stylesheet" href="https://maxcdn.bootstrapcdn.com/bootstrap/4.0.0/css/bootstrap.min.css" integrity="sha384-Gn5384xqQ1aoWXA+058RXPxPg6fy4IWvTNh0E263XmFcJlSAwiGgFAW/dAiS6JXm" crossorigin="anonymous">
        <link rel="stylesheet" href="css/style.css"/>
        <title>Tile Puzzle 3 | 3-Cycles</title>
    </head>
    <body>
        <div class="container">
            <div class="row mb-3 justify-content-center">
                <h1>Tile Puzzle 3 | 3-Cycles</h1>
            </div>
            <div class="row mb-3 justify-content-center">
                <p>Click 3 tiles to cycles their positions and solve the puzzle</p>
            </div>
            <div class="row justify-content-center">
                <div class="col-">
                    <div class="game-board d-flex flex-wrap"></div>
                </div>
            </div>
            <div class="row justify-content-center">
                <button id="reset-btn" type="button" class="mt-3 game-btn">Reset</button>
                &nbsp;
                <button id="scramble-btn" type="button" class="mt-3 game-btn">Scramble</button>
            </div>
        </div>
        <audio id="audio" src="assets/Success-sound-effect.mp3"></audio>
        <script src="scripts/puzzle3.js"></script>
    </body>
</html>

body {
    font-family: monospace, monospace;
}

.game-board {
    width: 408px;
    flex-wrap: wrap;
}

.game-tile {
    font-family: monospace, monospace; 
    font-size: 50px;
    color: white;
    width:100px;
    border: 1px solid white;
}

.game-btn {
    font-family: monospace, monospace;
    color: white;
    font-size: 36px;
    border: none;
    padding: 0 5px;
    background: green;
}

.game-btn:focus {
    outline: none;
}

.game-blue {
    background: blue;
}
.game-red {
    background: red;
}

```
\$\endgroup\$
1
  • \$\begingroup\$ How about starting with a scrambled board? Explaining what the cycle does (moves last clicked to first clicked position, etc.)? A trial counter? Congratulating player when solved? Future: Scoreboard? Competition against others? Leagues? Prizes? :) \$\endgroup\$
    – iAmOren
    Commented Oct 3, 2020 at 18:43

1 Answer 1

Reset to default
2
\$\begingroup\$

Stray console You have window.console.log( this.getAttribute( 'data-pos' ) );. They're useful for debugging, but remember to remove logs for production. Consider no-console.

n A variable named n is not all that informative. How about calling it totalNumberOfTiles instead? Also, rather than

for ( let i = 0; i < n; i++ ) {
initialTileArray.push( String( i + 1 ) );
}

You can start at 1 instead of 0 to make the logic a bit easier:

for ( let i = 1; i <= totalNumberOfTiles; i++ ) {
  initialTileArray.push( String( i ) );
}

Indentation A number of places in your code aren't properly indented, making it slightly harder to read than would be ideal. Consider an IDE which can indent automatically, like VSCode - that way the code can be made to look pretty without any manual effort at all on your part.

DOMContentLoaded? Keeping code free of unnecessary indentation can make it more readable - or, at least, some prefer it that way. If you feel the same, you can remove the DOMContentLoaded listener entirely - the <script> is at the bottom of the body already, so all elements will surely exist by then. You can also put the script in the <head> and do the same thing - all you need to do is give the script the defer attribute.

Biased sorting You use scrambledTileValues.sort(() => 0.5 - Math.random());. This is a biased algorithm. For a long article on this, see here; the results will not be altogether random. To sort, use a different method.

processTile bug The same tile can be selected more than once. For example, click 1, 1, then 2. The result is the 2 disappearing entirely; there are now two 1s on the board. Change processTile so that, if the clicked element is in selectedTiles already, do nothing. (Or deselect it, decrement clickNumber, and remove it from the array)

Pointer? Tiles are clickable. You might make this clearer by giving them cursor: pointer.

Color toggling Rather than having one class for blue and another for red, consider having just a base color (blue) with one class that gets toggled (red when on):

.game-tile {
  background: blue;
}
.selected {
  background: red;
}

That way, in the JS, you only have one class to toggle, not two.

Avoid var You're using ES6, which is great. In ES6, there's no reason to use var - use const instead (or let when you must reassign). Consider a linter which will prompt you to automatically fix it.

Or, even better, instead of iterating manually and messing with indicies, invoke the collection's iterator instead:

for ( var i = 0; i < tiles.length; i++ ) {
    config.push( tiles[i].innerHTML );
}

can be rewritten as

for (const tile of tiles) {
    config.push( tile.textContent );
}

Which leads to the next point:

Avoid innerHTML - unless you're deliberately inserting or retrieving HTML markup, it's faster, safer, and more semantically appropriate to use textContent instead.

Typo You refer to data-pos twice, and data-pas once. You probably wanted pos.

Also, for data attributes, rather than getAttribute, it can be nicer to use the dataset property instead: replace

const tilePos = this.getAttribute( 'data-pas' );

with

const tilePos = this.dataset.pos;

(or you could remove it entirely, since it isn't used anywhere else)

\$\endgroup\$
1
  • \$\begingroup\$ Awesome, thank you. That was exactly the kind of feedback I was looking for. I've implemented all of that now and will remember the points in future. \$\endgroup\$
    – Robin Andrews
    Commented Oct 5, 2020 at 19:57

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.