@@ -0,0 +1,87 @@
const statusDisplay = document.querySelector('.game--status');

let gameActive = true;
let currentPlayer = "X";
let gameState = ["", "", "", "", "", "", "", "", ""];

This comment has been minimized.

Copy link
Report content
@andyfry01

andyfry01 8 days ago

Owner Author

Instead of a "flat" list of empty strings representing coordinates on the board, why not use some formatting to shape them to look like the board itself?

A couple of strategic newlines and tabs will really aid comprehension:

let gameState = ["", "", "", 
             "", "", "", 
             "", "", ""];
Select a reply ctrl .

The content you are editing has changed. Please copy your edits and refresh the page.

Nothing to preview

Pick your reaction

@andyfry01

andyfry01 8 days ago
Author Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of a "flat" list of empty strings representing coordinates on the board, why not use some formatting to shape them to look like the board itself?

A couple of strategic newlines and tabs will really aid comprehension:

let gameState = ["", "", "", 
             "", "", "", 
             "", "", ""];
Select a reply ctrl .

The content you are editing has changed. Please copy your edits and refresh the page.

Nothing to preview

@andyfry01
Select a reply ctrl .

const winningMessage = () => `Player ${currentPlayer} has won!`;

This comment has been minimized.

Copy link
Report content
@andyfry01

andyfry01 8 days ago

Owner Author

Excellent practice to wrap things like this up into functions instead of hardcoded strings.

Select a reply ctrl .

The content you are editing has changed. Please copy your edits and refresh the page.

Nothing to preview

Pick your reaction

@andyfry01

andyfry01 8 days ago
Author Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent practice to wrap things like this up into functions instead of hardcoded strings.

Select a reply ctrl .

The content you are editing has changed. Please copy your edits and refresh the page.

Nothing to preview

@andyfry01
Select a reply ctrl .
const drawMessage = () => `Game ended in a draw!`;
const currentPlayerTurn = () => `It's ${currentPlayer}'s turn`;

statusDisplay.innerHTML = currentPlayerTurn();

const winningConditions = [
[0, 1, 2],
[3, 4, 5],
[6, 7, 8],
[0, 3, 6],
[1, 4, 7],
[2, 5, 8],
[0, 4, 8],
[2, 4, 6]
];
Comment on lines +13 to +22

This comment has been minimized.

Copy link
Report content
@andyfry01

andyfry01 3 minutes ago

Owner Author

Really like how you've implemented the winning conditions for this as a static set of triples representing a winning board. It's a very data-driven approach to checking for a win.

Checking for a winning condition could be implemented programmatically, and you could also write a function which generates this array of triples programmatically as well. If the domain we were working in was bigger, that might be a better choice, but seeing as we're working in a small context with this game, I think a hard-coded set of winning conditions is the perfect match for the task at hand 👍

Select a reply ctrl .

The content you are editing has changed. Please copy your edits and refresh the page.

Nothing to preview

Pick your reaction

@andyfry01

andyfry01 3 minutes ago
Author Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really like how you've implemented the winning conditions for this as a static set of triples representing a winning board. It's a very data-driven approach to checking for a win.

Checking for a winning condition could be implemented programmatically, and you could also write a function which generates this array of triples programmatically as well. If the domain we were working in was bigger, that might be a better choice, but seeing as we're working in a small context with this game, I think a hard-coded set of winning conditions is the perfect match for the task at hand 👍

Select a reply ctrl .

The content you are editing has changed. Please copy your edits and refresh the page.

Nothing to preview

@andyfry01
Select a reply ctrl .

function handleCellPlayed(clickedCell, clickedCellIndex) {
gameState[clickedCellIndex] = currentPlayer;
clickedCell.innerHTML = currentPlayer;
}

function handlePlayerChange() {
currentPlayer = currentPlayer === "X" ? "O" : "X";
statusDisplay.innerHTML = currentPlayerTurn();
}

function handleResultValidation() {
let roundWon = false;
for (let i = 0; i <= 7; i++) {
const winCondition = winningConditions[i];
let a = gameState[winCondition[0]];
let b = gameState[winCondition[1]];
let c = gameState[winCondition[2]];
if (a === '' || b === '' || c === '') {
continue;
}
if (a === b && b === c) {
roundWon = true;
break
}
}

if (roundWon) {
statusDisplay.innerHTML = winningMessage();
gameActive = false;
return;
}

let roundDraw = !gameState.includes("");
if (roundDraw) {
statusDisplay.innerHTML = drawMessage();
gameActive = false;
return;
}

handlePlayerChange();
}
Comment on lines +34 to +64

This comment has been minimized.

Copy link
Report content
@andyfry01

andyfry01 3 minutes ago

Owner Author

Firstly, some things I really like about this code:

  • It's wrapped up in a discrete function with a good name
  • It leverages early returns, and doesn't tangle its logic in a bunch of if/else trees, which can be hard to refactor down the line
  • It has nice whitespace to separate out the different parts, which helps us focus on the individual pieces of the function.

There are some things we can do which will make it even more readable:

  • Refactoring your for loop to a reduce function. for loops are good for looping tasks which require mutating state. In this context, we're checking the current board against the game's winning conditions to see whether to continue the game or whether a win or draw has occurred. This is a "data-driven" activity, and reduce is a tool which is much better suited for data-driven tasks.
  • Refactoring some boolean logic to use strings which represent game states.
  • Giving names to the logic checks you're performing, which will help us to more easily understand what exactly constitutes a win vs. a draw. When you've got variables to represent concepts, it makes things easier to read, seeing as you can rely on the variable name to communicate what's going on. Without a variable name, you have have to read the logic itself to understand what's going on. The logic in this context is not that hard to understand, but it nevertheless takes more time and effort to grok than a named variable would.

So, that being said, here's how I would refactor the code:

// At the top of the file, we can define some handy constants which represent game states: 
const IN_PROGRESS = "in progress"
const WIN = "win"
const DRAW = "draw"

// the refactored function itself
function handleResultValidation() {
const gameStatus = winningConditions.reduce((status, winCondition) => {
    const a = gameState[winCondition[0]]
    const b = gameState[winCondition[1]]
    const c = gameState[winCondition[2]]
    
    // a "vector" means either a column or a row
    const incompleteVector = (a === '' && b === '' && c === '')
    const filledVector = (a === b && b === c)
    const boardFilledUp = !gameState.includes("") 
    const gameFinished = filledVector && !incompleteVector

    if (status === WIN) {
        return WIN
    }

    if (boardFilledUp || status === DRAW) {
        return DRAW
    }
    
    return gameFinished ? WIN : IN_PROGRESS;
}, IN_PROGRESS)

return gameStatus === IN_PROGRESS ? handlePlayerChange() : endGame(gameStatus)
}
Select a reply ctrl .

The content you are editing has changed. Please copy your edits and refresh the page.

Nothing to preview

Pick your reaction

@andyfry01

andyfry01 3 minutes ago
Author Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Firstly, some things I really like about this code:

  • It's wrapped up in a discrete function with a good name
  • It leverages early returns, and doesn't tangle its logic in a bunch of if/else trees, which can be hard to refactor down the line
  • It has nice whitespace to separate out the different parts, which helps us focus on the individual pieces of the function.

There are some things we can do which will make it even more readable:

  • Refactoring your for loop to a reduce function. for loops are good for looping tasks which require mutating state. In this context, we're checking the current board against the game's winning conditions to see whether to continue the game or whether a win or draw has occurred. This is a "data-driven" activity, and reduce is a tool which is much better suited for data-driven tasks.
  • Refactoring some boolean logic to use strings which represent game states.
  • Giving names to the logic checks you're performing, which will help us to more easily understand what exactly constitutes a win vs. a draw. When you've got variables to represent concepts, it makes things easier to read, seeing as you can rely on the variable name to communicate what's going on. Without a variable name, you have have to read the logic itself to understand what's going on. The logic in this context is not that hard to understand, but it nevertheless takes more time and effort to grok than a named variable would.

So, that being said, here's how I would refactor the code:

// At the top of the file, we can define some handy constants which represent game states: 
const IN_PROGRESS = "in progress"
const WIN = "win"
const DRAW = "draw"

// the refactored function itself
function handleResultValidation() {
const gameStatus = winningConditions.reduce((status, winCondition) => {
    const a = gameState[winCondition[0]]
    const b = gameState[winCondition[1]]
    const c = gameState[winCondition[2]]
    
    // a "vector" means either a column or a row
    const incompleteVector = (a === '' && b === '' && c === '')
    const filledVector = (a === b && b === c)
    const boardFilledUp = !gameState.includes("") 
    const gameFinished = filledVector && !incompleteVector

    if (status === WIN) {
        return WIN
    }

    if (boardFilledUp || status === DRAW) {
        return DRAW
    }
    
    return gameFinished ? WIN : IN_PROGRESS;
}, IN_PROGRESS)

return gameStatus === IN_PROGRESS ? handlePlayerChange() : endGame(gameStatus)
}
Select a reply ctrl .

The content you are editing has changed. Please copy your edits and refresh the page.

Nothing to preview

@andyfry01
Select a reply ctrl .

function handleCellClick(clickedCellEvent) {
const clickedCell = clickedCellEvent.target;
const clickedCellIndex = parseInt(clickedCell.getAttribute('data-cell-index'));

This comment has been minimized.

Copy link
Report content
@andyfry01

andyfry01 8 days ago

Owner Author

This is an example of a variable name that I'd consider too verbose.

We're inside a function called handleCellClick, which takes as an argument an object called clickedCellEvent, which we're grabbing a clickedCell value from. There's a ton of context already for what we're working with – a cell that's been clicked on – so clickedCellIndex, while admirable in that the name strives to clearly communicate the concept that the index belongs to, is a bit excessive.

You could argue that index would be enough to communicate what's going on here, or maybe cellIndex is a happy medium between too verbose and not verbose enough.

Select a reply ctrl .

The content you are editing has changed. Please copy your edits and refresh the page.

Nothing to preview

Pick your reaction

@andyfry01

andyfry01 8 days ago
Author Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an example of a variable name that I'd consider too verbose.

We're inside a function called handleCellClick, which takes as an argument an object called clickedCellEvent, which we're grabbing a clickedCell value from. There's a ton of context already for what we're working with – a cell that's been clicked on – so clickedCellIndex, while admirable in that the name strives to clearly communicate the concept that the index belongs to, is a bit excessive.

You could argue that index would be enough to communicate what's going on here, or maybe cellIndex is a happy medium between too verbose and not verbose enough.

Select a reply ctrl .

The content you are editing has changed. Please copy your edits and refresh the page.

Nothing to preview

@andyfry01
Select a reply ctrl .

if (gameState[clickedCellIndex] !== "" || !gameActive) {
return;
}

handleCellPlayed(clickedCell, clickedCellIndex);
handleResultValidation();
}

function handleRestartGame() {
gameActive = true;
currentPlayer = "X";
gameState = ["", "", "", "", "", "", "", "", ""];
statusDisplay.innerHTML = currentPlayerTurn();
document.querySelectorAll('.cell').forEach(cell => cell.innerHTML = "");
}

document.querySelectorAll('.cell').forEach(cell => cell.addEventListener('click', handleCellClick));
document.querySelector('.game--restart').addEventListener('click', handleRestartGame);
Comment on lines +83 to +87

This comment has been minimized.

Copy link
Report content
@andyfry01

andyfry01 3 minutes ago

Owner Author

Things like query selectors for DOM elements fall into a category I refer to as "setup" or "bootstrapping" code. It's often helpful to place query selectors in their own section for organization, and to reference them by variable name instead of querying them directly inside a function. This aids in organization and separation of concerns between the setup for your app and the logic code which actually runs the functionality.

// Somewhere close to the top of the file, with the rest of your queried DOM elements: 
const cells = document.querySelectorAll('.cell')

// what you'd invoke on this line: 
cells.forEach(cell => cell.innerHTML = "");
Select a reply ctrl .

The content you are editing has changed. Please copy your edits and refresh the page.

Nothing to preview

Pick your reaction

@andyfry01

andyfry01 3 minutes ago
Author Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Things like query selectors for DOM elements fall into a category I refer to as "setup" or "bootstrapping" code. It's often helpful to place query selectors in their own section for organization, and to reference them by variable name instead of querying them directly inside a function. This aids in organization and separation of concerns between the setup for your app and the logic code which actually runs the functionality.

// Somewhere close to the top of the file, with the rest of your queried DOM elements: 
const cells = document.querySelectorAll('.cell')

// what you'd invoke on this line: 
cells.forEach(cell => cell.innerHTML = "");
Select a reply ctrl .

The content you are editing has changed. Please copy your edits and refresh the page.

Nothing to preview

@andyfry01
Select a reply ctrl .