-
Notifications
You must be signed in to change notification settings - Fork 17
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
Glib-w1-UsingAPIs #55
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 will advise you to also look closer to ex3 again as you technically changed the expected behaviour a bit. 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.
const value = Math.floor(Math.random() * 6) + 1; | ||
console.log(`Die value is now: ${value}`); | ||
|
||
if (roll >= 6) { |
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.
This alters the behaviour slightly
- Previously, the die could land on a valid value on the 6th roll.
- Now, it stops at the 6th roll, which means fewer valid outcomes.
|
||
// I have also changed condition 'if (roll > 6)' to if '(roll >= 6)', | ||
// so we check if next roll would exceed 6 before scheduling it. Otherwise | ||
// die rolls of the table only after 7 throws. |
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.
Mostly correct explanation, but there is a slight improvement to this answer. The key reason the problem stops is that a promise can only be settled once, meaning any calls to resolve() or reject() after the first one are ignored. Your use of return after reject() is the right fix to stop further execution, but that this is a control flow fix, not an inherent property of promises. Also, your change from roll > 6 to roll >= 6 slightly alters the intended behavior—double-check if that aligns with the original requirement.
// TODO Replace this comment by your explanation that was asked for in the assignment description. | ||
// The described problem happens because even if Promise.all gets rejected, | ||
// it doesn't cancel promises that have already begun executing. That's why | ||
// we see that some dice continue to roll even in case of rejected Promise. |
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. Can be rephrased that each that each promise (rollDie) is independent, so even if one promise rejects, the others continue to execute and finish.
.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
Finish assignment for week 1 of Using APIs module.