Completed crude tic-tac-toe game #5
Conversation
ROgbonna1
requested review from
ipc103 and
Keyairra-S-Wright
and removed request for
ipc103
|
|
||
| } | ||
|
|
||
| class Player{ |
There was a problem hiding this comment.
I'd recommend keeping the Player class separate from the Game class, similar to how the <Board/> component is separate from <PlayerOne/> and <PlayerTwo/> components. This will help keep the code base organized and maintainable.
There was a problem hiding this comment.
That makes perfect sense actually, I'll make that change
| const game = new Game() | ||
|
|
||
| const Board = () => { | ||
| const [box1, setBox1] = React.useState("") |
There was a problem hiding this comment.
Within board.jsx, this code base uses hooks and within the game.js module, the code base uses class components. There may be times when developers are updating apps from classes or hooks. Or there may be times when there is a specific tradeoff/benefit for switching between using hooks and class components in the same app.
Given that this code base was newly created by one developer, I'd recommend choosing one paradigm (hooks or classes) and sticking with it. Alternatively, in the Pull Request Summary, a developer could briefly highlight reasoning for switching between React "formats" so that other developers can better understand the decision.
| const [box6, setBox6] = React.useState("") | ||
| const [box7, setBox7] = React.useState("") | ||
| const [box8, setBox8] = React.useState("") | ||
| const [box9, setBox9] = React.useState("") |
There was a problem hiding this comment.
Setting state to an empty string nine times for each box is a valid approach. To make your code more even easier to reason about, consider refactoring so that the state of all boxes is set to an array.
There was a problem hiding this comment.
Oddly enough...I had no idea I could do that. Thank you!
| <div onClick={(e) => move(e)} className="box" id="box1">{box1}</div> | ||
| <div onClick={(e) => move(e)} className="box" id="box2">{box2}</div> | ||
| <div onClick={(e) => move(e)} className="box" id="box3">{box3}</div> | ||
| </section> | ||
| <section className="row"> | ||
| <div onClick={(e) => move(e)} className="box" id="box4">{box4}</div> | ||
| <div onClick={(e) => move(e)} className="box" id="box5">{box5}</div> | ||
| <div onClick={(e) => move(e)} className="box" id="box6">{box6}</div> | ||
| </section> | ||
| <section className="row"> | ||
| <div onClick={(e) => move(e)} className="box" id="box7">{box7}</div> | ||
| <div onClick={(e) => move(e)} className="box" id="box8">{box8}</div> | ||
| <div onClick={(e) => move(e)} className="box" id="box9">{box9}</div> |
There was a problem hiding this comment.
If there were a refactor to use an array as a default state, then there would be an added benefit of being able to map through the boxes in order to render the board.
There was a problem hiding this comment.
Definitely a change I'll be making
| if(this.posTracker['0'] === this.posTracker['1'] && this.posTracker['0'] === this.posTracker['2'] && this.posTracker['0'] !== ''){ | ||
| candidate % 2 === 0 ? game.playerOne.score += 1 : game.playerTwo.score += 1 | ||
| this.gameOver() | ||
| } |
There was a problem hiding this comment.
Each of the if statements are essentially the same except for the posTracker position. Consider refactoring in order to reduce the amount of repetitive code here.
| game.checkBoard(game.turn) | ||
|
|
||
| game.turn += 1 | ||
| return game.turn |
There was a problem hiding this comment.
The if statements are nearly identical. It appears that the difference is only which box state is set. In that case, this can be refactored into something like:
| return game.turn | |
| if(game.turn %2 !==0}{ | |
| boxStates[box](<PlayerOne/>) | |
| } else { | |
| boxStates[box](<PlayerTwo/>) | |
| } | |
| document.getElementById(box).removeAttribute("onClick"); | |
| //etc. |
No description provided.