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

add task solution #448

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

add task solution #448

wants to merge 2 commits into from

Conversation

siefimov
Copy link

No description provided.

Copy link

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

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

Thank you for your hard work on implementing the "Bulls & Cows" game! 🎉 While your solution is mostly correct and meets the task requirements, there are a few critical issues that need to be addressed before we can approve the pull request:

  1. Variable Typo: There's a typo in the variable name nymberToGuessDigits which should be corrected to numberToGuessDigits to avoid runtime errors.

  2. Unnecessary Condition: The check !uniqueDigits.has('0') in the generateRandomNumber function is redundant since the number is already ensured to be a valid 4-digit number starting from 1000. This can be safely removed.

  3. Readline Interface Handling: The readLine.close() method is being called multiple times, which can lead to errors. It should only be called once after the game logic is complete. Please restructure the code to allow for repeated guesses until the correct number is guessed, and then close the readline interface.

Once these issues are resolved, your code will be ready for approval. Keep up the great work, and don't hesitate to reach out if you have any questions! 😊

✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨

🔄 Fix the issues, commit the changes, and then re-request my review using the Re-request review button in the Reviewers tab.

src/app.js Outdated
Comment on lines 24 to 30
if (bulls === 4) {
readLine.close();
} else {
readLine.close();
}

readLine.close();

Choose a reason for hiding this comment

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

The readLine.close() method is called multiple times here. It should only be called once after the game logic is complete. Consider restructuring the code to allow for repeated guesses until the correct number is guessed, and then close the readline interface.

if (randomNumber.toString().length === 4) {
const uniqueDigits = new Set(randomNumber.toString());

if (uniqueDigits.size === 4 && !uniqueDigits.has('0')) {

Choose a reason for hiding this comment

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

The condition !uniqueDigits.has('0') is unnecessary because the number is already ensured to be a 4-digit number starting from 1000. You can remove this check.

let cows = 0;

const userInputDigits = numberToGuess.toString().split('');
const nymberToGuessDigits = userInput.toString().split('');

Choose a reason for hiding this comment

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

There is a typo in the variable name nymberToGuessDigits. It should be numberToGuessDigits to match the intended logic and avoid runtime errors.

Copy link

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

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


Great job on implementing the "Bulls & Cows" game! 🎉 Your code meets the task requirements and constraints, and you've done well in generating a random number, validating user input, and calculating bulls and cows. However, there are a few critical issues that need attention:

  1. Variable Naming: Ensure that userInputDigits is derived from userInput, and numberToGuessDigits is derived from numberToGuess. This will prevent logical errors in the getBullsAndCows function.

  2. Game Logic: The readLine.close() method should only be called once the correct number is guessed. Currently, it might terminate the game prematurely if the user input is invalid. Adjust this logic to allow users to try again until they guess correctly.

These issues are important for the functionality of your code, so please address them in future iterations. Keep up the good work and continue learning from your experiences! 🚀

✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨

function startBullsAndCows() {
readLine.question('Enter a 4-digit number: ', (userInput) => {
if (!checkIsValidUserInput(userInput)) {
readLine.close();

Choose a reason for hiding this comment

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

Calling readLine.close() here will terminate the program if the user input is invalid, preventing further attempts. Consider removing this line to allow the user to try again.

const { bulls } = getBullsAndCows(userInput, numberToGuess);

if (bulls === 4) {
readLine.close();

Choose a reason for hiding this comment

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

The readLine.close() method should only be called once the correct number is guessed. Ensure that this logic is correctly implemented to prevent premature termination of the game.

Comment on lines +19 to +20
const userInputDigits = numberToGuess.toString().split('');
const numberToGuessDigits = userInput.toString().split('');

Choose a reason for hiding this comment

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

The variables userInputDigits and numberToGuessDigits are swapped. userInputDigits should be derived from userInput, and numberToGuessDigits should be derived from numberToGuess. Please correct the assignment to avoid logical errors.

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