5
\$\begingroup\$

I am a beginner and was hoping for some honest feedback on my js code. I struggled quite a bit on this, I did ask gpt to explain some concepts which I find it’s good for but wrote it all myself.

// variable to store array of players 
const players = ['X', 'O'];
// declaring currentplayer with array index 
let currentPlayer = players[0];
let xScore = 0;
let oScore = 0;

// store cell class into cellElements 
const cellElements = document.querySelectorAll('.cell');
const resetBtn = document.querySelector('.reset');
const resultElem = document.querySelector('#results');

// array of possible win combinations  
const winningMoves = [
    [0, 1, 2],
    [3, 4, 5],
    [6, 7, 8],
    [0, 3, 6],
    [1, 4, 7],
    [2, 5, 8],
    [0, 4, 8],
    [2, 4, 6]   
];

// for of loop to loop through each cell and add event listener 
for (const cellElem of cellElements) {
    cellElem.addEventListener('click', handleClick);
}

// function to execute click in each cell 
function handleClick(event) {
    const clickedCellElem = event.target;

    // if else condition checking if cell is empty then inputting current play and switching between X and O     
    if (clickedCellElem.textContent === '') {
        clickedCellElem.textContent = currentPlayer;

        // calling checkWin function after each move 
        if (checkWin()) {
            resultElem.textContent = (`${currentPlayer} wins!`);
        // disable cell if win
            disableCell();
        } else if (checkDraw()) {
            disableCell();
            resultElem.textContent = (`It's a draw!`);
        }

        // if else condition to switch between the current player array index 
        if (currentPlayer === players[0]) {
            currentPlayer = players[1];
        } else {
            currentPlayer = players[0];
        }
    }
}

// check for a draw after no winning combinations 
function checkDraw(){
    for (const cellElem of cellElements)
    if (cellElem.textContent === '') {
        return false
    }
    return true
}


// function to execute when player wins by having 3 cells in a row with X or O
function checkWin() {
    // loops through each element in winning combinations 
    for (const winningMove of winningMoves) {

    // extracts individual elements into three separate variables to store the index of winnings combinations array 
        const [cellIndex1, cellIndex2, cellIndex3] = winningMove;
    // DOM grabbing html cell ID content of cells which corresponds to each winning combination index in array
        const cell1Content = document.getElementById(`cell-${cellIndex1}`).textContent;
        const cell2Content = document.getElementById(`cell-${cellIndex2}`).textContent;
        const cell3Content = document.getElementById(`cell-${cellIndex3}`).textContent;
    
        // condition to compare each 3 cells are the same to declare winner 
        if (cell1Content !== '' && cell1Content === cell2Content && cell2Content === cell3Content) {
            //increments player score 
            if (currentPlayer === players[0]) {
                xScore++;
                // displays the score getting DOM id element 
                document.getElementById("X-score").textContent = `[X] ${xScore}`;
            } else {
                oScore++;
                document.getElementById("O-score").textContent = `[O] ${oScore}`;
            }

            return true;
        }
    }

    return false;
}

// function to reset the game
function resetGame() {
    //loop through cell elements 
    for (const cellElem of cellElements) {
        //reset the cells to empty string 
        cellElem.textContent = '';
        // add event listener again after cell disabled 
        cellElem.addEventListener('click', handleClick);
    }
    // reset result message 
    resultElem.textContent = '';
}

// Attach the resetGame function to resetBtn
resetBtn.addEventListener('click', resetGame);

//function to disbable the cells after win 
function disableCell() {
    for (const cellElem of cellElements) {
        cellElem.removeEventListener('click', handleClick);
    }
}
body {
    display: flex;
    flex-direction: column;
    align-items: center;
    justify-content: center;
    margin: 0;
    font-family: Arial, sans-serif;
    background-color: #000;
    margin-top: 7rem;
}

h1 {
    font-size: 2em;
    margin-bottom: 30px;
    /* color: yellow; */
    color: #FFF01F;
    font-family: "Retro Gaming";
    border: 2px solid #88be19;
    padding: 10px;
    border-image: url("data:image/svg+xml;charset=utf-8,%3Csvg width='100' height='100' viewBox='0 0 100 100' fill='none' xmlns='http://www.w3.org/2000/svg'%3E %3Cstyle%3Epath%7Banimation:stroke 5s infinite linear%3B%7D%40keyframes stroke%7Bto%7Bstroke-dashoffset:776%3B%7D%7D%3C/style%3E%3ClinearGradient id='g' x1='0%25' y1='0%25' x2='0%25' y2='100%25'%3E%3Cstop offset='0%25' stop-color='%232d3561' /%3E%3Cstop offset='25%25' stop-color='%23c05c7e' /%3E%3Cstop offset='50%25' stop-color='%23f3826f' /%3E%3Cstop offset='100%25' stop-color='%23ffb961' /%3E%3C/linearGradient%3E %3Cpath d='M1.5 1.5 l97 0l0 97l-97 0 l0 -97' stroke-linecap='square' stroke='url(%23g)' stroke-width='3' stroke-dasharray='388'/%3E %3C/svg%3E") 1;
}

header {
    display: flex;
    align-items: center;
    justify-content: space-between;
}

h2 {
    font-size: 1.5em;
    margin-bottom: 30px;
    color: #39FF14;
    font-family: "Retro Gaming";
}

.board {
    display: grid;
    grid-template-columns: 1fr 1fr 1fr;
    gap: 2px;
    margin-bottom: 2rem;
    max-width: 100%;

}

.cell {
    width: 110px;
    height: 110px;
    border: 1px solid #FFF01F;
    box-sizing: border-box;
    display: flex;
    align-items: center;
    justify-content: center;
    font-size: 4.5rem;
    cursor: pointer;
    font-family: "Retro Gaming";
    color: blue;
}

.cell:hover {
    background-color: #1a1b1c;
    opacity: 1.0;
}

h3 {
    border: 2px solid #88be19;
    color: #39FF14;
    padding: 13px;
    text-align: center;
    font-family: "Retro Gaming";
    font-size: 20px;
    margin-bottom: 8px;
}

.player-score {
    padding: 5px;
    padding-top: 20px;
    margin: 5px;
    font-family: "Retro Gaming";
    color: #39FF14;
}

.reset {
    margin-bottom: 15rem;
    color: #f55b1d;
    font-family: "Retro Gaming";
    background-color: #000;
    border-radius: 2rem;
    cursor: pointer;
    font-size: 18px;
}

.reset:hover {
    transform: scale(1.2);
}
<!DOCTYPE html>
<html lang="en">

<head>
    <meta charset="UTF-8">
    <meta name="viewport" content="width=device-width, initial-scale=1.0">
    <link rel="stylesheet" href="style.css">
    <link href="https://db.onlinewebfonts.com/c/4c19fc875e7ba1e6831129de3ab5ac0b?family=Retro+Gaming" rel="stylesheet">
    <link rel="stylesheet" href="https://cdnjs.cloudflare.com/ajax/libs/animate.css/4.1.1/animate.min.css" />
    <title>Document</title>
</head>

<body>
    <h1 class="animate__animated animate__bounce">Tic-Tac-Toe</h1>
    <header class="container">
        <div class="player-score" id="X-score">[X] 0</div>
        <h3>Scoreboard</h3>
        <div class="player-score" id="O-score">[O] 0</div>
    </header>
    <h2 id="results"></h2>
    <div class="board">
        <div class="cell" id="cell-0"></div>
        <div class="cell" id="cell-1"></div>
        <div class="cell" id="cell-2"></div>
        <div class="cell" id="cell-3"></div>
        <div class="cell" id="cell-4"></div>
        <div class="cell" id="cell-5"></div>
        <div class="cell" id="cell-6"></div>
        <div class="cell" id="cell-7"></div>
        <div class="cell" id="cell-8"></div>
    </div>
<div class="button-container">
    <button class="reset">New Game</button>
</div>
    <script src="game.js"></script>
</body>

</html>

\$\endgroup\$
0

1 Answer 1

2
\$\begingroup\$

There are only a few things I would change with your JS

if/else vs switch

if (checkWin()) {
  resultElem.textContent = (`${currentPlayer} wins!`);
  // disable cell if win
  disableCell();
} else if (checkDraw()) {
  disableCell();
  resultElem.textContent = (`It's a draw!`);
}

This part is repetitive and low-performing. If you have 2 if statements then you are nearly always better off with a switch statement. In this specific case, I would change the code completely:

switch (true) {
  case checkWin():
    endGame(`${currentPlayer} wins!`);
    break;
  case checkDraw():
    endGame(`It's a draw!`);
    break;
  default:
    break;    
}

function endGame(text) {
  disableCell();
  resultElem.textContent = text;
}

This will perform faster for most browsers and you remove the repetitive code as in both cases you write a result message and disable all cells.

if/else vs ternary vs modulus

You have another if/else statement block that could be simplified as it is repetitive again.

if (currentPlayer === players[0]) {
  currentPlayer = players[1];
} else {
  currentPlayer = players[0];
}

The most obvious change would be the usage of a ternary conditional operator:

currentPlayer = players[currentPlayer === players[0] ? 1 : 0];

You use a ternary to check the condition within the assignment with following syntax:

condition ? true : false

const players = ['X', 'O'];
let currentPlayer = players[0];

function changePlayer() {
  currentPlayer = players[currentPlayer === players[0] ? 1 : 0];
  
  console.clear();
  console.log(currentPlayer);
}
<button onclick="changePlayer()">change player</button>

Another method would be the usage of a modulus here. However you would need to change the logic here to output the index number with currentPlayer:

const players = ['X', 'O'];
let currentPlayer = 0;

function changePlayer() {
  currentPlayer = (currentPlayer + 1) % players.length;
  
  console.clear();
  console.log(players[currentPlayer]);
}
<button onclick="changePlayer()">change player</button>

return true/false vs ! (not) statement

You have a line with an if statement to return the negated condition.

if (cellElem.textContent === '') {
  return false
}
return true

In this case, you check a condition and want to return true if it is return false. The same vice-versa.
It can be simplified;

return !cellElem.textContent === '';

A condition will already return true or false So you simply negate the condition with a not or !.

You could also do a double negation on textContent.length to check if the element is empty or not:

function checkTextContent(cellElem) {
  return !!cellElem.textContent.length;
}

console.log(checkTextContent(document.querySelector('#element-1')));
console.log(checkTextContent(document.querySelector('#element-2')));
<div id="element-1">Test</div>
<div id="element-2"></div>


Overall you have a good understanding of how to use if/else conditions but lack the knowledge about other more suitable methods and when to use them.

\$\endgroup\$
2
  • \$\begingroup\$ Instead of using %, toggling between two players could also be done using 1 - currentPlayer \$\endgroup\$ Feb 6 at 15:45
  • 1
    \$\begingroup\$ @TobySpeight I totally agree. This is coming from more abstract cases where you potentially have more than 2 players. Or instances that you want to infinitely iterate through an array. I hope that he just remembers when he has to iterate though a larger array that you can also use the modulus. \$\endgroup\$
    – tacoshy
    Feb 6 at 15:54

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.