2
\$\begingroup\$

I have made the tic-tac-toe game as part of The Odin Project course.

I think the code I have written just works but is not properly optimized for performance and it could be made simpler.

This is script.js file

    const gameboard = () =>
{
    //setting up the player and the array of board
    let board = ['','','','','','','','',''];
    let player = (symbol) =>
    {
        getsymbol = () => symbol;    
        return{getsymbol}
    }
    let player_one = player('X');
    let player_two = player('O');

    //DOM - selection of blocks.
    var blocks = document.querySelectorAll('.block');
    var resultdiv = document.querySelector('.result');
    var counter = 1;
    var cntr = 0;

    function play()
    {
        blocks.forEach((block)=>
        {
            block.addEventListener('click',  ()=>
            {
                if (cntr == 1)
                {
                    console.log('refresh the page to continue');
                }
                else
                {
                    if(block.textContent == '')
                    {
                        if(counter % 2 == 1)
                        {
                            block.textContent = player_one.getsymbol();
                        }
                        else if (counter % 2 == 0)
                        {
                            block.textContent = player_two.getsymbol();
                        }                    
                        board[parseInt(block.getAttribute('value'))] = block.textContent;

                        if (counter>4)
                        {
                            check();
                        }
                        //console.log(counter);  
                        counter++;
                    }
                    else if(block.textContent != '')
                    {
                        console.log('You already selected that.');
                    }
                }
            })
        })
    }
        

    //for inserting the board to the screen 
    function display()
    {   
        blocks.forEach((block)=>
        {
            for (let i = 0; i < board.length; i++) 
            {
                block.innerHTML = board[i];
                console.log('hello')
            }
        })
    }
    function check()
    {
        var winconditions = [
            [0,1,2],
            [3,4,5],
            [6,7,8],
            [0,3,6],
            [1,4,7],
            [2,5,8],
            [0,4,8],
            [2,4,6]
        ]
        var conditioncounter = 1;
        for(let i = 0; i < winconditions.length; i++ )
        {            
            if(board[winconditions[i][0]] == board[winconditions[i][1]] && board[winconditions[i][1]] == board[winconditions[i][2]] && board[winconditions[i][0]] != '' && board[winconditions[i][0]] == player_one.getsymbol())
            {
                cntr = 1;

                let result = document.createElement('h2');
                result.textContent = 'Player 1 Win  ';

                let refresh = document.createElement('button')
                refresh.innerHTML = 'Refresh';
                refresh.setAttribute('onClick', 'window.location.reload();');
                result.appendChild(refresh);
                
                resultdiv.appendChild(result);
                break;                
            }
            else if(board[winconditions[i][0]] == board[winconditions[i][1]] && board[winconditions[i][1]] == board[winconditions[i][2]] && board[winconditions[i][0]] != '' && board[winconditions[i][0]] == player_two.getsymbol())
            {
                cntr = 1;
                let result = document.createElement('h2');
                result.textContent = 'Player 2 Win';

                let refresh = document.createElement('button')
                refresh.innerHTML = 'Refresh';
                refresh.setAttribute('onClick', 'window.location.reload();');
                result.appendChild(refresh);

                resultdiv.appendChild(result);                
                break;
            }
            
            else if(board.includes(''))
            {
                continue;
            }
            else 
            {
                let result = document.createElement('h2');
                result.textContent = 'Draw Game';

                let refresh = document.createElement('button')
                refresh.innerHTML = 'Refresh';
                refresh.setAttribute('onClick', 'window.location.reload();');
                result.appendChild(refresh);

                resultdiv.appendChild(result);                
                break;
            }
            
        }

    }  
    play();
    return{board};
}

let gb  = gameboard();

This is the HTML file

<!DOCTYPE html>
<html lang="en">
<head>
    <title>Tic-tac-toe</title>
    <link rel="stylesheet" href="style.css">
    <script src="script.js" defer></script>
</head>
<body>
    <br><br><br><br>
    <div id="container">
        <div class="block" value='0'></div>
        <div class="block" value='1'></div>
        <div class="block" value='2'></div>
        <div class="block" value='3'></div>
        <div class="block" value='4'></div>
        <div class="block" value='5'></div>
        <div class="block" value='6'></div>
        <div class="block" value='7'></div>
        <div class="block" value='8'></div>
    </div>
    <div class="result">
        
    </div>
</body>
</html>

And this is the CSS file

*, *::before, *::after{
    box-sizing: border-box;
    font-family: -apple-system, BlinkMacSystemFont, 'Segoe UI', Roboto, Oxygen, Ubuntu, Cantarell, 'Open Sans', 'Helvetica Neue', sans-serif;
    font-weight: normal;
}
body {
    padding: 0;
    margin: 0;
    background-color: black;
}

#container
{
    display: grid;
    grid-template-columns: repeat(3, 40px);
    grid-template-rows: repeat(3,40px);
    border: 0px solid black;
    justify-content: center;
    align-content: center;
    min-height: 0vh;
    grid-row-gap: 8px;
    grid-column-gap: 8px;
    margin: auto;
    width: 50%;    
    padding: 70px 0;
    text-align: center;
}
.block
{
    line-height: 40px;
    text-align: center;
    border: 0px solid black;
    background-color: white;
}
.result
{
    color: white;
    text-align: center;
}

Any help on how to increase performance, reduce and improve code and better programming practices would be greatly appreciated.

\$\endgroup\$
1
  • \$\begingroup\$ In Javascript, you may run into syntax issues if you put curly brackets on different lines, especially with some upcoming features that are being added to Javascript. There are technical reasons for this, but it boils down to wanting to add new keywords while keeping backwards compatibility. If you really care, see here for one example: github.com/tc39/proposal-pattern-matching/issues/196 \$\endgroup\$ Jul 7, 2021 at 4:47

1 Answer 1

1
\$\begingroup\$

Review

This review only looks at the JavaScript.

Style

  • Opening { on the same line.

  • else on the same line as closing }

  • Declare constants with const

  • JavaScript naming convention is camelCase. Avoid using snake_case

  • Be consistent with semicolons.

  • Avoid using continue as it breaks the flow.

  • Avoid using break when not needed.

  • Space before the ( for tokens. Example if (, for (, while ( and so on.

  • Use strict equality === rather than ==. Same with inequality !== rather than !=

  • Use for of when possible rather than for;;

  • Do not test a condition twice.

    Example you have...

    if (counter % 2 == 1) {
        // code
    } else if (counter % 2 == 0) { // if counter % 2 is not 1 it must be 0
    

    can be...

       if (counter % 2) {  // will evaluate to true
          // code
       } else {  // no need to check counter % 2 is 0 
    

Design

The code is overly complex due to repeated code and repeated conditional checks. Your lack of experience is part of the problem, but do keep at it.

As a beginner I suggest you use the directive "use strict"; at the top of each JS block or file.

The player object can be simplified. All it does is return the symbol X, O so just store the string for each player.

The end game check is retesting the same condition many times. Move the drawn game test out of the loop. The test for a matching row need only be done once per iteration. If a row is complete then test which player has that row rather than test each row for each player. (See rewrite)

Rewrite

The rewrite simplifies your original code.

The rewrite is untested.

I could not find a call to display however I rewrote that function and left it in.

"use strict";
;(() => {
    const board = ["", "", "", "", "", "", "", "", ""];
    const playerOne = "X", playerTwo = "O";
    const blocks = document.querySelectorAll(".block");
    const resultdiv = document.querySelector(".result");
    const winConditions = [[0,1,2], [3,4,5], [6,7,8], [0,3,6], [1,4,7], [2,5,8],[0,4,8], [2,4,6]];  
    var counter = 1;
    var gameOver = false;

    const cellClick = (block) => () => { /* returns a function to use as listener */
        if (gameOver) {
            console.log("Refresh the page to continue");
        } else {
            if (!block.textContent) {
                block.textContent = counter % 2 ? playerOne : playerTwo;
                board[block.value] = block.textContent;
                counter++ > 4 && check();
            } else {
                console.log("Invalid move");
            }
        }
    };
    function play() {
        blocks.forEach(block => block.addEventListener("click", cellClick(block)));
    }        

    function display() {       /* This function is never used??? */
        blocks.forEach((block)=> {
            for (const cell of board.length)  { block.textContent = cell }
        });
    }
    function displayResult(text) {
        const result = Object.assign(document.createElement("h2"), {textContent: text});
        const refreshBtn = Object.assign(document.createElement("button"), {
            textContent: "Refresh", onClick: () => location.reload()
        });
        result.appendChild(refreshBtn);                 
        resultdiv.appendChild(result);
    }
    function check() {
        for (const winPos of winConditions) {            
            if (board[winPos[0]] == board[winPos[1]] && board[winPos[1]] == board[winPos[2]] && board[winPos[0]] !== "") {              
                gameOver = true;
                displayResult("Player " + (board[winPos[0]] === playerOne ? "1" : "2") + " Wins");
                return;
            } 
        }
        if (!board.includes("")) {
            gameOver = true;
            displayResult("Game drawn");
        }               
    }  
    play();
})();

\$\endgroup\$
1
  • \$\begingroup\$ Thank You for your input and re-write. I have implemented all the changes you suggested. The only thing that does not work as you have written is function play(). For some reason the cellClick(block) referencing does not work properly and throws weird errors in the board. To overcome that I had to write the function inside the play() function itself. It works perfectly everywhere else. \$\endgroup\$ Jul 7, 2021 at 5:50

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.