-
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
KHIRO-w1-UsingAPIs. #50
base: main
Are you sure you want to change the base?
Conversation
…using Promises and using Promise.all .then .catch
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.
if (num >= 10 && num <= 99){ | ||
return resolve("This is a double digit number!"); | ||
} else{ | ||
return reject( new Error( `Expected a double digit number but got ${num} `) ) |
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.
You don’t need to use return before resolve() or reject() because resolve() and reject() already determine the control flow.
} | ||
|
||
// ! Do not change or remove the code below | ||
if (process.env.NODE_ENV !== 'test') { | ||
main(); | ||
} | ||
|
||
// TODO Replace this comment by your explanation that was asked for in the assignment description. | ||
// The previous problem while using callback no longer occurs. the die used to keep rolling even after error ( going of the table ), but now it stops once the error occurs. |
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 work here.
Consider cleaning up the function and removing leftover callback references in comments.
Your explanation is correct in stating that the problem no longer happens, but the "why" can be explained better. It's not just because of promises but because a promise only resolves or rejects once, preventing further execution.
@@ -43,4 +42,4 @@ if (process.env.NODE_ENV !== 'test') { | |||
main(); | |||
} | |||
|
|||
// TODO Replace this comment by your explanation that was asked for in the assignment description. | |||
// Because using map creates 5 promises. each promise runs separately from the others in asynchronous way. once the promise runs cant be cancelled and it will keep going even after the rejection happens. |
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
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 time and your amazing insights. very helpful tips.
Solved week 1 of the APIs models refactoring the callbacks functions using Promises and using Promise.all .then .catch