finished #435
finished #435
Conversation
There was a problem hiding this comment.
Thank you for your hard work on this project! While your implementation of the "Bulls & Cows" game is mostly correct and meets many of the task requirements, there are a few critical issues that need to be addressed before we can approve the pull request:
-
Bulls and Cows Calculation: The logic for calculating bulls and cows has some errors. The condition for checking bulls should directly compare digits at the same index, and the condition for cows should ensure that the digit is not already counted as a bull. These logical errors can lead to incorrect game results.
-
User Input Validation: The
checkIsUniqueNumbersfunction returns true prematurely and modifies the array during iteration, which can lead to incorrect validation results. This needs to be corrected to ensure accurate input validation. -
ESLint Configuration: The 'no-proto' rule is currently disabled. If this is not intentional, consider setting it to 'warn' or 'error' to enforce the rule and maintain code quality.
-
GitHub Actions: It's recommended to update the 'setup-node' action to 'v3' to benefit from the latest features and security updates.
Please address these issues to ensure the functionality and quality of your code. Once resolved, your solution will be in great shape! Keep up the great work, and don't hesitate to reach out if you need any help. 😊
✨ 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.
.eslintrc.js
Outdated
| }, | ||
| rules: { | ||
| 'no-proto': 0 | ||
| 'no-proto': 0, |
There was a problem hiding this comment.
The 'no-proto' rule is set to 0, which disables it. If this is not intentional, consider setting it to 'warn' or 'error' to enforce the rule.
| - run: npm test | ||
| - uses: actions/checkout@v2 | ||
| - name: Use Node.js ${{ matrix.node-version }} | ||
| uses: actions/setup-node@v1 |
There was a problem hiding this comment.
Consider using 'actions/setup-node@v3' instead of 'v1' to ensure you are using the latest features and security updates.
src/modules/checkIsValidUserInput.js
Outdated
| for (let i = 0; i < arrayValue.length; i++) { | ||
| const numberToCheck = arrayValue[i]; | ||
|
|
||
| arrayValue.splice(i, 1); | ||
|
|
||
| if (arrayValue.includes(numberToCheck)) { | ||
| return false; | ||
| } | ||
|
|
||
| return true; | ||
| } |
There was a problem hiding this comment.
The loop in checkIsUniqueNumbers returns true prematurely after the first iteration. It should only return true after checking all digits. Also, modifying the array with splice during iteration can lead to incorrect results. Consider using a different approach to check for duplicates without altering the array.
src/modules/checkIsValidUserInput.js
Outdated
| const checkIsUniqueNumbers = (value) => { | ||
| const arrayValue = `${value}`.split(''); | ||
|
|
||
| if (arrayValue.length !== 4 && typeof value !== 'number') { |
There was a problem hiding this comment.
The condition here should use '||' instead of '&&'. It should check if the length is not 4 or if the type is not a number.
src/modules/getBullsAndCows.js
Outdated
| if ( | ||
| numberToGuessArr.includes(userInputArr[i]) && | ||
| numberToGuessArr.indexOf(userInputArr[i]) === i |
There was a problem hiding this comment.
The condition for checking bulls is incorrect. It should check if the digits are the same and in the same position, but the current logic checks if the digit exists in the array and is in the same position, which is redundant. Consider simplifying this condition to directly compare the digits at the same index.
src/modules/getBullsAndCows.js
Outdated
| numberToGuessArr.indexOf(userInputArr[i]) === i | ||
| ) { | ||
| result.bulls += 1; | ||
| } else if (numberToGuessArr.includes(userInputArr[i])) { |
There was a problem hiding this comment.
The condition for checking cows should ensure that the digit is not already counted as a bull. Consider adding a check to ensure the digit is not in the correct position before counting it as a cow.
There was a problem hiding this comment.
Great job on making progress with your code! 🎉 There are a few critical issues that need attention to ensure the functionality and quality of your solution:
-
Bulls and Cows Calculation: The logic needs adjustment to prevent counting a digit as a cow if it has already been counted as a bull. This will ensure accurate results.
-
User Input Validation: The
checkIsUniqueNumbersfunction returns true prematurely. Thereturn true;statement is inside the loop, causing the function to return true after checking only the first digit. Move this statement outside the loop to ensure all digits are checked for uniqueness. -
GitHub Actions: Consider updating the 'setup-node' action from 'v1' to 'v3' to take advantage of the latest features and security updates.
Your ESLint configuration and random number generation logic are well set up. Keep up the good work! Remember, addressing these issues will help you improve your code quality and functionality. Keep learning and refining your skills! 🚀
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
| - run: npm test | ||
| - uses: actions/checkout@v2 | ||
| - name: Use Node.js ${{ matrix.node-version }} | ||
| uses: actions/setup-node@v1 |
There was a problem hiding this comment.
Consider updating the 'setup-node' action to 'v3' to benefit from the latest features and security updates.
| // console.log(hiddenNumber); | ||
|
|
||
| rl.question('Write your number? ', (userValue) => { | ||
| const isValidUserInput = checkIsValidUserInput(userValue); |
There was a problem hiding this comment.
Ensure that the checkIsValidUserInput function correctly validates the input. The checkIsUniqueNumbers function currently returns true prematurely, which can lead to incorrect validation results. Make sure it checks all digits for uniqueness before returning true.
| return false; | ||
| } | ||
|
|
||
| return true; |
There was a problem hiding this comment.
The return true; statement is inside the loop, causing the function to return true after checking only the first digit. Move this statement outside the loop to ensure all digits are checked for uniqueness before returning true.
No description provided.