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

Pine: Kris & Lia #60

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

Pine: Kris & Lia #60

wants to merge 12 commits into from

Conversation

lgaetano
Copy link

No description provided.

Copy link

@jbieniosek jbieniosek left a 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;

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.

Copy link
Author

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!

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;

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;

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).

Copy link
Author

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';

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]) {

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.

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