-
Notifications
You must be signed in to change notification settings - Fork 17
Sertan-w1-UsingAPIs #49
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job on the assignment. I left some clarification comments on some of the explanations, however, there is no blocker for me to approve this assignment. If you have any questions, you can reach me on Slack.
Refactored the code with using Promises and the problem is not occur anymore. | ||
When we did not return the resolve/reject statements the code will continue to execute | ||
even if it was resolve/reject. Adding return will prevent the further executions. | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your explanation is mostly correct, but a small improvement: The main reason the issue no longer occurs is that a promise can only be settled once, so any additional calls to resolve() or reject() are ignored. Returning helps stop further function execution, but it's the nature of Promises that ensures only one resolution.
If you remove the return, you will notice the function still works ok and you get a correct result in the returned value here Success! Die settled on ${value}
but you might still see some console logs because the function keeps executing even though its result is already determined. The problem we originally had is the result is changing if the function keeps executing, we still get a new value returned even if the die already rolled off the table.
@@ -43,4 +43,5 @@ if (process.env.NODE_ENV !== 'test') { | |||
main(); | |||
} | |||
|
|||
// TODO Replace this comment by your explanation that was asked for in the assignment description. | |||
// When one of the promises rejected the other dice continue rolling. | |||
// Because each rollDie is an independent async function and rejecting one does not effect the others. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice
.then((value) => { | ||
results.push(value); | ||
return rollDie(5); | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good job.
One small tip in JS to stay DRY (Don't repeat yourself) is that you can refactor this to outside function e.g
const pushAndRoll = (dice) => (value) => {
results.push(value);
return rollDie(dice);
};
And then inside your chain you can just do something like this
.then(pushAndRoll(2))
.then(pushAndRoll(3))
.then(pushAndRoll(4))
.then(pushAndRoll(5))
...rest of code
Dear Abdullah, thank you very much for your time to review my assignment and for your tips :) |
No description provided.