-
Notifications
You must be signed in to change notification settings - Fork 10
YANA_SENIUK-w1-UsingAPIs #37
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?
YANA_SENIUK-w1-UsingAPIs #37
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.
Thank you for your work, Yana. You have correctly solved all exercises; I've left a couple of comments regarding your answers to the two questions about the Promise
behaviour and an alternative/advanced solution to the last exercise.
number >= 10 && number <= 99 | ||
? resolve('This is a double digit number!') | ||
: reject(new Error(`Expected a double digit number but got '${number}'`)); | ||
}); |
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.
That's a nice use of the ternary operator here!
|
||
No, the problem does not occur with the promise-based version. | ||
We have a better control of setTimeout, avoid callback hell, easier to read and scale | ||
*/ |
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.
While what you say is correct, it doesn't really explain the technical reason behind the change :)
When a promise rejects, it stops the execution of the function. So after reject
is called on line 22, additional throws are not executed, and the loop stops there.
The new promise resolves when all listed promises are resolved, and the array of their results becomes its result. | ||
Promise.all never stop promises. This method return promise which was rejected or all all promises are resolved and return their results. | ||
Other promises are still running, cause if it was started it will continue till resolve or rejected state. | ||
|
||
Promise.All just ignores their result in this case. |
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.
I think you got it right, although you chose a somewhat roundabout way to explain it. It could also be summarized as:
- Each
rollDie(number)
function is an independent async process. Promise.all()
does not cancel any promises; it just waits.- So even if one promise rejects, the rest keep going.
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 solution is good, and the benefit is that it's easy to understand what's happening or troubleshoot. On the other hand, it includes some code repetition and would be harder to scale if you had a larger number of dice to throw.
If you wanted to streamline the code, you could define a function factory that returns a new .then()
handler for each die, capturing the die number in a closure.
const pushAndRoll = (dice) => (value) => {
results.push(value);
return rollDie(dice);
};
Then, you could do:
[...]
return rollDie(1)
.then(pushAndRoll(2))
.then(pushAndRoll(3))
.then(pushAndRoll(4))
.then(pushAndRoll(5))
.then((value) => {
results.push(value);
return results;
});
No description provided.