-
Notifications
You must be signed in to change notification settings - Fork 80
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
Pine: Kris & Lia #60
base: main
Are you sure you want to change the base?
Pine: Kris & Lia #60
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work on this project Kris & Lia! This project is green.
036, 147, 258 vertical winners | ||
048, 246 diagonal winners */ | ||
|
||
let winner = null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is an interesting bug in this function! The code fails the test that checks the bottom row of x's. The test setup goes as follows: 6 - x, 1 - o, 8 - x, 2 - o, 7 - x, which results in 2 o's in the top row and 3 x's in the bottom row. The if-else sequence to find the winner fails at this point, because line 94 is true! For values 3, 4 and 5, ticDict is undefined, so they are equal! As a result, winner is set to undefined.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh my goodness. Thank you for figuring this one out--we were stumped!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haha, cool bug!!! Thanks for pointing it out :-)
squares.forEach((row) => { | ||
row.map((square) => { | ||
if (square.value == '') { | ||
square.disabled = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Neat addition!
src/App.js
Outdated
row.forEach((square) => { | ||
if (square.id === squareID && square.value === '') { | ||
square.value = turn; | ||
ticDict[squareID] = turn; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This variable isn't being used correctly as a state variable. A state variable should be treated as if it is const at all times, and changes should be made through the corresponding set state function. To change a state var, make a copy, make the changes to the copy, and then call the set state function and pass in the changed copy. This can cause some issues if you want to use the new version right away, but you can bypass this by using the copied version (ie, pass the copied version to checkForWinner).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Jasmine, thank you for the feedback. Can you clarify which variable isn't being used correctly as a state variable here? Do you mean that we shouldn't be writing directly to ticDict? We should be writing to copy of ticDict and then using a set function to update it?
src/App.js
Outdated
} else if (ticDict[2] == ticDict[4] && ticDict[4] == ticDict[6]) { | ||
winner = ticDict[2]; | ||
} else if (Object.keys(ticDict).length === 9) { | ||
winner = 'tie'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
src/App.js
Outdated
winner = ticDict[2]; | ||
} else if (ticDict[0] == ticDict[4] && ticDict[4] == ticDict[8]) { | ||
winner = ticDict[0]; | ||
} else if (ticDict[2] == ticDict[4] && ticDict[4] == ticDict[6]) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a long sequence of hardcoded checks - I recommend thinking about how you could shorten this with loops.
No description provided.