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

mc - guesswho #350

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

mc - guesswho #350

wants to merge 2 commits into from

Conversation

mirelcac
Copy link

No description provided.

Comment on lines 251 to +252
const selectQuestion = () => {
console.log(selectQuestion)

Choose a reason for hiding this comment

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

Hello Mirela,

Nice Work Mirela!

your code looks mostly fine, but there are a few minor issues and improvements that can be made:

In the selectQuestion function, you have a console.log statement with console.log(selectQuestion). You should log the actual values or the result of the function, like console.log(currentQuestion), to see the selected question.

if (personToCheck === secret.name) {
winOrLoseText.innerHTML = `That's correct!!! You won! Congratulations!`;
} else {
personToConfirm !== secret.name;

Choose a reason for hiding this comment

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

In the checkMyGuess function, you have an unnecessary line: personToConfirm !== secret.name;. It doesn't serve any purpose and can be removed.

Comment on lines 292 to +313
const filterCharacters = (keep) => {
const { category, value } = currentQuestion
// Show the correct alert message for different categories
if (category === 'accessories') {
if (keep) {
alert(
`Yes, the person wears ${value}! Keep all people that wears ${value}`
`Yes, the person wears ${value}! Keep all people who wear ${value}`
)
} else {
alert(
`No, the person doesn't wear ${value}! Remove all people that wears ${value}`
`No, the person doesn't wear ${value}! Remove all people who wear ${value}`
)
}
} else if (category === 'other') {
// Similar to the one above
} else {
if (keep) {
// alert popup that says something like: "Yes, the person has yellow hair! Keep all people with yellow hair"
alert(`Yes, the person wears ${value}! Keep all people who wear ${value}`
)
} else {
alert(
`No, the person doesn´t wear ${value}! Remove all people who wear ${value}`
)
}
} else if (category === 'hair') {

Choose a reason for hiding this comment

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

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});
}

// FILTER BY CATEGORY TO KEEP/REMOVE BASED ON THE KEEP VARIABLE//
charactersInPlay = charactersInPlay.filter((person) => person[category].includes(value) === keep);

// Invoke a function to redraw the board with the remaining people.
generateBoard();
};

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