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 crude tic-tac-toe game #5

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions tic-tac-toe-app/App.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
const container = document.querySelector('.container')



const App = () => {
return (
<div>
<div>A tic-tac-toe game</div>
<Board />
</div>
)
}


ReactDOM.render(<App />, container);
5 changes: 0 additions & 5 deletions tic-tac-toe-app/App.jsx

This file was deleted.

72 changes: 72 additions & 0 deletions tic-tac-toe-app/components/board.jsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
const container = document.querySelector('.container')
const game = new Game()

const Board = () => {
const [box1, setBox1] = React.useState("")

Choose a reason for hiding this comment

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

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 [box2, setBox2] = React.useState("")
const [box3, setBox3] = React.useState("")
const [box4, setBox4] = React.useState("")
const [box5, setBox5] = React.useState("")
const [box6, setBox6] = React.useState("")
const [box7, setBox7] = React.useState("")
const [box8, setBox8] = React.useState("")
const [box9, setBox9] = React.useState("")

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

Oddly enough...I had no idea I could do that. Thank you!


const move = (e) => {

const boxStates = {
box1: setBox1,
box2: setBox2,
box3: setBox3,
box4: setBox4,
box5: setBox5,
box6: setBox6,
box7: setBox7,
box8: setBox8,
box9: setBox9,
}
const box = e.target.id
if(game.turn % 2 !== 0){
boxStates[box](<PlayerOne/>)
document.getElementById(box).removeAttribute("onClick");
console.log(box)
game.playerOne.makeMove(parseInt(box[3]) - 1)
console.log(game.board)
game.checkBoard(game.turn)

game.turn += 1
return game.turn
Copy link

@Keyairra-S-Wright Keyairra-S-Wright May 3, 2020

Choose a reason for hiding this comment

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

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:

Suggested change
return game.turn
if(game.turn %2 !==0}{
boxStates[box](<PlayerOne/>)
} else {
boxStates[box](<PlayerTwo/>)
}
document.getElementById(box).removeAttribute("onClick");
//etc.

}
if(game.turn % 2 === 0){
boxStates[box](<PlayerTwo/>)
document.getElementById(box).removeAttribute("onClick");
console.log(box)
game.playerTwo.makeMove(parseInt(box[3]) - 1)
console.log(game.board)
game.checkBoard(game.turn)
game.turn += 1
return game.turn
}
}
return (
<div>
<section className="row">
<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>
Comment on lines +54 to +66

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

Definitely a change I'll be making

</section>
</div>
)
}


11 changes: 11 additions & 0 deletions tic-tac-toe-app/components/players.jsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
const PlayerOne = () => {
return (
<div className="x-icon">X</div>
)
}

const PlayerTwo = () => {
return (
<div className="o-icon">O</div>
)
}
83 changes: 83 additions & 0 deletions tic-tac-toe-app/game.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,83 @@
class Game{
constructor(){
this.playerOne = new Player()
this.playerTwo = new Player()
this.playerOne.move = 'X'
this.playerTwo.move = 'O'
this.board = ['', '', '', '', '', '', '', '', '']
this.posTracker = {}
this.turn = 1
this.winner = null
}

checkBoard(candidate){

for(let position in this.board){
this.posTracker[position] = this.board[position]
}
//Checks each row to see if a player won
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()
}
Comment on lines +19 to +22

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

I'll work on this!

if(this.posTracker['3'] === this.posTracker['4'] && this.posTracker['3'] === this.posTracker['5'] && this.posTracker['3'] !== ''){
candidate % 2 === 0 ? game.playerOne.score += 1 : game.playerTwo.score += 1
this.gameOver()
}
if(this.posTracker['6'] === this.posTracker['7'] && this.posTracker['6'] === this.posTracker['8'] && this.posTracker['6'] !== ''){
candidate % 2 === 0 ? game.playerOne.score += 1 : game.playerTwo.score += 1
this.gameOver()
}
//Checks each diagonal row to see if a player won
if(this.posTracker['0'] === this.posTracker['4'] && this.posTracker['0'] === this.posTracker['8'] && this.posTracker['0'] !== ''){
candidate % 2 === 0 ? game.playerOne.score += 1 : game.playerTwo.score += 1
this.gameOver()
}
if(this.posTracker['2'] === this.posTracker['4'] && this.posTracker['2'] === this.posTracker['6'] && this.posTracker['2'] !== ''){
candidate % 2 === 0 ? game.playerOne.score += 1 : game.playerTwo.score += 1
this.gameOver()
}
//Checks each column to see if a player won
if(this.posTracker['0'] === this.posTracker['3'] && this.posTracker['0'] === this.posTracker['6'] && this.posTracker['0'] !== ''){
candidate % 2 === 0 ? game.playerOne.score += 1 : game.playerTwo.score += 1
this.gameOver()
}
if(this.posTracker['1'] === this.posTracker['4'] && this.posTracker['1'] === this.posTracker['7'] && this.posTracker['1'] !== ''){
candidate % 2 === 0 ? game.playerOne.score += 1 : game.playerTwo.score += 1
this.gameOver()
}
if(this.posTracker['2'] === this.posTracker['5'] && this.posTracker['2'] === this.posTracker['8'] && this.posTracker['2'] !== ''){
candidate % 2 === 0 ? game.playerOne.score += 1 : game.playerTwo.score += 1
this.gameOver()
}
//Checks to see if there is a draw
if(candidate >= 9){
console.log('Draw game')
alert('Draw Game')
location.reload()
}

}

gameOver(){
this.board = ['', '', '', '', '', '', '', '', '']
if(game.turn % 2 !== 0){
alert('Player One Wins')
location.reload()
Devonte202 marked this conversation as resolved.
Show resolved Hide resolved
return 1
}
alert('Player Two Wins')
location.reload()
return 2
}

}

class Player{
Copy link

@Keyairra-S-Wright Keyairra-S-Wright May 2, 2020

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

That makes perfect sense actually, I'll make that change

constructor(){
this.score = 0
}
makeMove(index){
game.board.splice(index, 1, this.move)
}
}
7 changes: 5 additions & 2 deletions tic-tac-toe-app/index.html
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,17 @@
<meta charset="UTF-8">
<meta name="viewport" content="width=device-width, initial-scale=1.0">
<link rel="stylesheet" href="styles.css">
<title>Document</title>
<title>React Tac Toe</title>
</head>
<body>
<main class="container"></main>

<script crossorigin src="https://unpkg.com/react@16/umd/react.development.js"></script>
<script crossorigin src="https://unpkg.com/react-dom@16/umd/react-dom.development.js"></script>
<script src="https://cdnjs.cloudflare.com/ajax/libs/babel-standalone/6.26.0/babel.js"></script>
<script type="text/babel" src="App.jsx"></script>
<script type="text/javascript" src="game.js"></script>
<script type="text/babel" src="./components/players.jsx"></script>
<script type="text/babel" src="./components/board.jsx"></script>
<script type="text/babel" src="App.js"></script>
</body>
</html>
Loading