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

Guess who game #335

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

Guess who game #335

wants to merge 14 commits into from

Conversation

mikaeloling
Copy link

Almost finished, has a few issues when restarting game.

Copy link

@mirelcac mirelcac left a comment

Choose a reason for hiding this comment

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

All in all the game is playable, and works as it should! My favourite part was the timer as I did not manage to add that. There are some shortcuts that could be made to make the code shorter, so I recommend removing and adding to see if it effects the code in the end of the week if there is time. It looks like you understood the assignment and put an effort into it, great job on this project!


//declaring a clock counter for the website, it starts at 2 minutes and counts down to 0
// In minute format, sarting at 02:00
let time = 120

Choose a reason for hiding this comment

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

I like the timer function, as I did not do that myself! Made the page more alive and interactive. Stressed me out through ;)

}

// This function should be invoked when you click on 'Find Out' button.
const checkQuestion = () => {
const { category, value } = currentQuestion
const { category, value } = currentQuestion // MISSING PIECE THAT FINALLY MADE THE FILTER WORK..currentQuestion is an object with two properties: category and value. Declaring these two variables and assigning them to the properties of currentQuestion object makes it possible to use them in the if statement below.

Choose a reason for hiding this comment

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

I found this complicated, you explain it well here

Comment on lines +380 to +382
`Yes, the person has ${value} hair! Keep all people that have ${value} hair.`
)
charactersInPlay = charactersInPlay.filter((person) => person[category] === value)

Choose a reason for hiding this comment

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

From playing your game a few times, it looks like you did the filtering correctly. However, the values could be paid more attention to. None of the caracters match the value red hair, while there is no value for hidden hair or orange and purple hair. This makes it harder to filter away the wrong options.


findOutButton.addEventListener('click', checkQuestion)

playAgainButton.addEventListener('click', start)

Choose a reason for hiding this comment

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

I see you mentioned some of this in your readme - the play again (after guessing) and the restart (while playting) buttons work well (you have the event listeners set up), however the game does not reset. The scroll down menu should go back to "please choose an option" and timer should start. The secret person does change however. Perhaps the end of my code can help you here - notice the start function being called once more:

//Restart game
restartButton.addEventListener('click', start)
//Play again from the win/ lose site
playAgainButton.addEventListener('click', () => {
winOrLose.style.display = "none"
start()

//Game board should be rendered on the screen
generateBoard();
setSecret();
//startTimer(); // can't get this to work together with findout dropdown filter

Choose a reason for hiding this comment

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

It could be helpful to console log when the start function is being called, as it is easier to spot errors in restart / play again

alert(
`No, the person doesn't have ${value} hair! Remove all people that have ${value} hair.`
)
charactersInPlay = charactersInPlay.filter((person) => person[category] !== value)
}
}

Choose a reason for hiding this comment

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

Stenli left this comment for me - I found it useful as I used the same approach as you, so passing it on:
the readability of your filterCharacters function can be improved by avoiding the repetition of alert messages. You can create a single alert message and use template literals to make the code cleaner.

like the following :

const filterCharacters = (keep) => {
const { category, value } = currentQuestion;
let message;

if (category === 'accessories') {
message = person wears ${value};
} else if (category === 'other') {
message = person wears ${value};
} else if (category === 'hair') {
message = person has ${value} hair;
} else if (category === 'eyes') {
message = person has ${value} eyes;
}

if (keep) {
alert(Yes, the ${message}! Keep all people who ${message});
} else {
alert(No, the ${message}! Remove all people who ${message});
}

// 4. Hide the game board
board.style.display = 'none';

Choose a reason for hiding this comment

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

Do you need both of these? I think display flex should overwrite the board

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.

2 participants