Solution #419
Solution #419
Conversation
There was a problem hiding this comment.
Great job on your code so far! 🚀 However, there's a potential performance issue that needs your attention. The use of array method includes inside a forEach loop can lead to quadratic complexity, which may cause problems for larger inputs. Try to optimize this part. Don't worry, you're doing well, and you're very close to the finish line! Keep it up! 😊
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
|
|
||
| function playGame(question = '') { | ||
| terminal.question(question, (userInput) => { | ||
| if (!checkIsValidUserInput(userInput)) { |
There was a problem hiding this comment.
Code quality issue: It's a good practice to keep your functions small and focused on doing one thing. The playGame function is doing multiple things: validating the user input, incrementing the count, getting the result, logging the result, checking the result, and deciding what to do next. Consider breaking it down into smaller functions to improve readability and maintainability.
| console.log( | ||
| 'It is not valid number! Input the 4 dight number, without duplicates!', | ||
| ); | ||
| playGame('Try again!'); |
There was a problem hiding this comment.
Bug: The 'Try again!' message is not very informative. It would be better to tell the user exactly what they did wrong and how they can correct it.
|
|
||
| const result = getBullsAndCows(userInput, guessNumber); | ||
|
|
||
| console.log(result, '(Try', count, ')'); |
There was a problem hiding this comment.
Code quality issue: The way you are logging the result is a bit confusing. It would be better to use a template string to make it clearer. For example: console.log(${result} (Try ${count}));
|
|
||
| if (result.bulls === 4) { | ||
| console.log( | ||
| 'Congratulation!!! You win!!! Number is... ', |
There was a problem hiding this comment.
Code quality issue: Consider using a template string for the win message as well. For example: console.log(Congratulation!!! You win!!! Number is... ${guessNumber} [From ${count} try!]);
| } else { | ||
| const { bulls, cows } = result; | ||
|
|
||
| if (bulls === 0 && cows === 0) { |
There was a problem hiding this comment.
Recommendation: The conditions on lines 38, 42, and 46 are a bit confusing. It would be clearer to use a switch statement or an if-else-if chain instead. This would also allow you to remove the redundant conditions on lines 47-51.
| const availableNums = [1, 2, 3, 4, 5, 6, 7, 8, 9]; | ||
| const result = []; | ||
|
|
||
| const getRandomN = () => Math.floor(Math.random() * availableNums.length); |
There was a problem hiding this comment.
It's better to use more descriptive function names. getRandomN doesn't give a clear understanding of what this function does. Consider renaming it to something like getRandomIndex.
|
|
||
| const getRandomN = () => Math.floor(Math.random() * availableNums.length); | ||
|
|
||
| result.push(availableNums.splice(getRandomN(), 1)[0]); |
There was a problem hiding this comment.
The use of splice method here can be confusing. It's better to avoid changing the original array. Consider using a different approach to select a random element from the array without modifying it.
| const getRandomN = () => Math.floor(Math.random() * availableNums.length); | ||
|
|
||
| result.push(availableNums.splice(getRandomN(), 1)[0]); | ||
| availableNums.unshift(0); |
There was a problem hiding this comment.
The use of unshift here is not clear. If you need to add 0 to the list of available numbers, why not include it in the original array? Also, it's not clear why 0 is added after the first number is selected. This could lead to confusion for other developers reading your code.
| result.push(availableNums.splice(getRandomN(), 1)[0]); | ||
| } | ||
|
|
||
| return Number(result.join('')); |
There was a problem hiding this comment.
Converting an array of numbers to a string and then back to a number can be unnecessary and may lead to confusion. Consider returning the array itself or joining the array elements into a string and returning that string.
| userInputArr.forEach((el, i) => { | ||
| if (el === numberToGuessArr[i]) { | ||
| result.bulls++; | ||
| } else if (numberToGuessArr.includes(el)) { | ||
| result.cows++; | ||
| } | ||
| }); |
There was a problem hiding this comment.
Consider using a different strategy to improve the performance of this code. For instance, you could create a map (an object in JavaScript) to store the count of each digit in numberToGuessArr. Then, for each digit in userInputArr, you can check if it's in the map and update the counts of bulls and cows accordingly. This will reduce the time complexity to linear.
No description provided.