Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Completed Game #4

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

Devonte202
Copy link

Known bugs

  • Squares are double clickable
  • Time Machine buttons can give a player back to back turns as a result of turn being globally mutated
  • Turn is changed on every rerender rather than every specific turn

Known areas for optimization

  • Lack of pure functions can lead to possible unintended side effects
  • Global variables polluting namespace
  • Component views do not respond well to changing screen size

Copy link

@ipc103 ipc103 left a comment

Choose a reason for hiding this comment

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

Hey @Devonte202 - really nice work with this Tic Tac Toe game! I like your choice to use useReducer as well as the context for the score. I left a bunch of comments inline - feel free to respond if you have any questions or thoughts!


const [step, setStep] = useState(0)

const deleteParadox = (clickedButton) => {
Copy link

Choose a reason for hiding this comment

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

I love this function name, but I'm a little bit confused about what this function is doing. Is this erasing part of the history when you travel back?

import TimeMachine from './TimeMachine.js'
import { ScoreContext } from '../contexts/ScoreContext.js'

const board = [null,null,null,null,null,null,null,null,null]
Copy link

Choose a reason for hiding this comment

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

You can using Array(9).fill(null) to do this so you don't have to hard-code it out.


const board = [null,null,null,null,null,null,null,null,null]
let turn = 0
let timeSteps = []
Copy link

Choose a reason for hiding this comment

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

To respond to your global variable comment - I'd probably just nest these inside of the component :-) Another option would be to create a custom hook called useBoard or something that could hold these initial values, plus the useReducer call below.



const Board = () => {
const [score, setScore] = useContext(ScoreContext)
Copy link

Choose a reason for hiding this comment

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

👍 I really like how you separate the score from the other board state via the context. Keeps your concerns nice and should scale well.

return state
}
}
const [state, dispatch] = useReducer(boardReducer, board)
Copy link

Choose a reason for hiding this comment

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

👍 👍 for useReducer in this case - I think this is a really good choice for storing the state. This lets you handle the time travel relatively easily.

Copy link

Choose a reason for hiding this comment

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

You could probably even let your timeSteps state be a part of this as well. That way, you could handle that with separate actions instead of with useEffect. You could even check for the winner each time the board is updated here in the reducer.

@@ -0,0 +1,9 @@
import React from 'react'

const PlayerTwo = () => {
Copy link

Choose a reason for hiding this comment

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

This is kind of cool - means that you can change the player value if needed relatively easily.


useEffect(() => {
timeSteps.push(state)
turn += 1
Copy link

Choose a reason for hiding this comment

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

Instead of using a global variable for your turn, I'd store it as a piece of application state. That way, you can update it when you update the other moves and when you go back in time, you can reset to the correct player's turn.

<div className="boardArea">
<div className="board">
{state.map((box, index) => {
if(box === "X"){
Copy link

Choose a reason for hiding this comment

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

Looking at this again, it might be easier to just render 'X' or 'O' directly. I think having the separate Player components probably adds some un-needed complexity.

}
else {
return (
<div key={index} className="box" id={index} onClick={() =>dispatch({type: 'move', index: index})}></div>
Copy link

Choose a reason for hiding this comment

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

To make sure that the square can only be selected once, you could have this onClick function check to see if it's an occupied square or not before firing the dispatch.

@ipc103 ipc103 removed their assignment Apr 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants