Accelerate - Karolina Benitez #3
Conversation
There was a problem hiding this comment.
Nice job!
Your tests are all passing, and your code is well-organized.
One thing to focus on is how to work with container types in React. Your logic for updating squares and checking for the winner definitely work, but you're side-stepping the squares state a little bit. The squares reference never gets updated, so it never triggers a re-render. Fortunately, other pieces of state do get updated, resulting in the re-render we need. Check the comments in App.js for details.
The overall structure and approach looks great though!
| @@ -13,6 +13,7 @@ | |||
|
|
|||
| # misc | |||
| .DS_Store | |||
| .env | |||
There was a problem hiding this comment.
I'm curious what prompted the inclusion of .env here. Were you tracking something in the .env file?
| return <button | ||
| className="square" | ||
| onClick={()=>{onClickCallback(id)}} | ||
| id={id} |
There was a problem hiding this comment.
There's not really a need to set the id attribute here. The anonymous function in your onClick handler is already holding onto the id value through closure semantics.
| // Component to alert a parent | ||
| // component when it's clicked on. | ||
|
|
||
| const Square = ({ id, value, onClickCallback } ) => { |
| let destructuredSquare = [] | ||
|
|
||
| destructuredSquare.push(...squares[0]) | ||
| destructuredSquare.push(...squares[1]) | ||
| destructuredSquare.push(...squares[2]) | ||
|
|
||
| return destructuredSquare |
There was a problem hiding this comment.
You could use a loop to generalize this sort of function, or if you know that there are always only 3 inputs, we could rewrite this as:
return [...squares[0], ...squares[1], ...squares[2]];| // squares is a 2D Array, but | ||
| // you need to return a 1D array | ||
| // of square components | ||
| const squaresArray = destructureMatrix(squares) |
There was a problem hiding this comment.
Nice job writing a method to help flatten the matrix! Also, check out the flat() method that exists on JS arrays.
| if(squares[i][j].id === squareID && squares[i][j].value === ''){ | ||
| squares[i][j].value = currentPlayer | ||
| updateCurrentPlayer(currentPlayer, setCurrentPlayer) | ||
| setSquares(squares) |
There was a problem hiding this comment.
It turns out this call has no effect. You are processing the squares data directly, flipping the space corresponding to the clicked square to the current player. But since the squares reference is the same reference that React was already holding onto, this call to setSquares doesn't trigger a re-render. But we do see a re-render, so what's going on?
Since the current player gets flipped, that triggers the re-render. The squares collection held by React was updated, so when it renders the next view, we see the changes, but the setSquares had nothing to do with it.
Recall the general pattern for container objects (arrays, dictionaries, and general objects) in React involves making a copy of the existing values into a new value and updating the value held by the copy. This allows React to see the change, and also ensures that all the code that was rendered together has a consistent view of the data that was part of the render.
| for(let i = 0; i < squares.length; i++){ | ||
| for(let j = 0; j < squares[i].length; j++){ | ||
| if(squares[i][j].id === squareID && squares[i][j].value === ''){ | ||
| squares[i][j].value = currentPlayer |
There was a problem hiding this comment.
Rather than updating the squares contents directly, it's generally preferable with React to make a copy and update the copy.
| } | ||
| if(possibilities[i][0] === possibilities[i][1] && possibilities[i][1] === possibilities[i][2]){ | ||
| if(possibilities[i][0] === 'x'){ | ||
| x += 1 |
There was a problem hiding this comment.
We know that we're calling this function after every move, so as soon as we find three in a row (so long as it's not three blanks) we can stop looking. There should never be a case where we find more than 1 three in a row of xs and os.
| setWinner(null) | ||
| } | ||
| if(x === 0 && o === 0 && !empty) | ||
| setWinner('Tie') |
There was a problem hiding this comment.
I would personally be more inclined to introduce an additional boolean piece of state specifically to to track whether the game was tied. If you really prefer using the same winner state, then I'd suggest making a const to hold the special string indicating the tie condition. The same constant could then be used later when you're building the game result string.
Making a single const value helps protect us from typos in strings, which is always a risk with magic values.
| {winner === 'Tie' ? <h2 className='tieGame'>The game is tied!</h2> : ''} | ||
| {winner === null ? <h2>The current player is {currentPlayer}</h2> : ''} | ||
| {winner === 'o' || winner === 'x'? <h2>Winner is {winner.toLowerCase()}</h2> : ''} |
There was a problem hiding this comment.
I would be inclined to pull this out of the markup here and into code that runs before the JSX return stuff, or into a helper function. There's sort of two things going on here: figuring out the className, and figuring out the h2 contents. Calculating those ahead of time before getting here can help make that logic more visible, and make the markup here a little clearer.
No description provided.