-
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?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
## Test Summary | ||
|
||
**Mentors**: For more information on how to review homework assignments, please refer to the [Review Guide](https://github.com/HackYourFuture/mentors/blob/main/assignment-support/review-guide.md). | ||
|
||
### 3-UsingAPIs - Week1 | ||
|
||
| Exercise | Passed | Failed | ESLint | | ||
|-----------------------|--------|--------|--------| | ||
| ex1-johnWho | 9 | - | ✓ | | ||
| ex2-checkDoubleDigits | 11 | - | ✓ | | ||
| ex3-rollDie | 7 | - | ✓ | | ||
| ex4-pokerDiceAll | 7 | - | ✓ | | ||
| ex5-pokerDiceChain | 5 | - | ✓ | |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,54 +9,47 @@ Full description at: https://github.com/HackYourFuture/Assignments/tree/main/3-U | |
- Does the problem described above still occur? If not, what would be your | ||
explanation? Add your answer as a comment to be bottom of the file. | ||
------------------------------------------------------------------------------*/ | ||
|
||
// TODO Remove callback and return a promise | ||
export function rollDie(callback) { | ||
// Compute a random number of rolls (3-10) that the die MUST complete | ||
const randomRollsToDo = Math.floor(Math.random() * 8) + 3; | ||
console.log(`Die scheduled for ${randomRollsToDo} rolls...`); | ||
|
||
const rollOnce = (roll) => { | ||
// Compute a random die value for the current roll | ||
const value = Math.floor(Math.random() * 6) + 1; | ||
console.log(`Die value is now: ${value}`); | ||
|
||
// Use callback to notify that the die rolled off the table after 6 rolls | ||
if (roll > 6) { | ||
// TODO replace "error" callback | ||
callback(new Error('Oops... Die rolled off the table.')); | ||
} | ||
|
||
// Use callback to communicate the final die value once finished rolling | ||
if (roll === randomRollsToDo) { | ||
// TODO replace "success" callback | ||
callback(null, value); | ||
} | ||
|
||
// Schedule the next roll todo until no more rolls to do | ||
if (roll < randomRollsToDo) { | ||
setTimeout(() => rollOnce(roll + 1), 500); | ||
} | ||
}; | ||
|
||
// Start the initial roll | ||
rollOnce(1); | ||
export function rollDie() { | ||
return new Promise((resolve, reject) => { | ||
const randomRollsToDo = Math.floor(Math.random() * 8) + 3; | ||
console.log(`Die scheduled for ${randomRollsToDo} rolls...`); | ||
|
||
const rollOnce = (roll) => { | ||
// Compute a random die value for the current roll | ||
const value = Math.floor(Math.random() * 6) + 1; | ||
console.log(`Die value is now: ${value}`); | ||
if (roll > 6) { | ||
reject(new Error('Oops... Die rolled off the table.')); | ||
} | ||
|
||
// Use callback to communicate the final die value once finished rolling | ||
if (roll === randomRollsToDo) { | ||
resolve(value); | ||
} | ||
|
||
// Schedule the next roll todo until no more rolls to do | ||
if (roll < randomRollsToDo) { | ||
setTimeout(() => rollOnce(roll + 1), 500); | ||
} | ||
}; | ||
|
||
rollOnce(1); | ||
}); | ||
} | ||
|
||
function main() { | ||
// TODO Refactor to use promise | ||
rollDie((error, value) => { | ||
if (error !== null) { | ||
console.log(error.message); | ||
} else { | ||
console.log(`Success! Die settled on ${value}.`); | ||
} | ||
}); | ||
rollDie() | ||
.then((value) => console.log(`Success! Die settled on ${value}.`)) | ||
.catch((error) => console.log(error)); | ||
} | ||
|
||
// ! 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. | ||
/* | ||
|
||
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 commentThe 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 :) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -27,9 +27,9 @@ exercise file. | |
import { rollDie } from '../../helpers/pokerDiceRoller.js'; | ||
|
||
export function rollDice() { | ||
// TODO Refactor this function | ||
const dice = [1, 2, 3, 4, 5]; | ||
return rollDie(1); | ||
const dicePromises = dice.map((number) => rollDie(number)); | ||
return Promise.all(dicePromises); | ||
} | ||
|
||
function main() { | ||
|
@@ -43,4 +43,11 @@ if (process.env.NODE_ENV !== 'test') { | |
main(); | ||
} | ||
|
||
// TODO Replace this comment by your explanation that was asked for in the assignment description. | ||
/* | ||
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. | ||
Comment on lines
+47
to
+51
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
|
||
|
||
*/ |
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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, you could do:
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
*** Unit Test Error Report *** | ||
|
||
PASS .dist/3-UsingAPIs/Week1/unit-tests/ex1-johnWho.test.js | ||
api-wk1-ex1-johnWho | ||
✅ should exist and be executable (2 ms) | ||
✅ should have all TODO comments removed | ||
✅ `getAnonName` should not contain unneeded console.log calls (1 ms) | ||
✅ should call `new Promise()` | ||
✅ should take a single argument (1 ms) | ||
✅ `resolve()` should be called with a one argument | ||
✅ `reject()` should be called with a one argument | ||
✅ should resolve when called with a string argument (1 ms) | ||
✅ should reject with an Error object when called without an argument (1 ms) | ||
|
||
Test Suites: 1 passed, 1 total | ||
Tests: 9 passed, 9 total | ||
Snapshots: 0 total | ||
Time: 0.897 s, estimated 1 s | ||
Ran all test suites matching /C:\\Users\\senyu\\hackyourfuture\\Assignments-cohort52\\.dist\\3-UsingAPIs\\Week1\\unit-tests\\ex1-johnWho.test.js/i. | ||
No linting errors detected. | ||
No spelling errors detected. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,23 @@ | ||
*** Unit Test Error Report *** | ||
|
||
PASS .dist/3-UsingAPIs/Week1/unit-tests/ex2-checkDoubleDigits.test.js | ||
api-wk1-ex2-checkDoubleDigits | ||
✅ should exist and be executable (2 ms) | ||
✅ should have all TODO comments removed (1 ms) | ||
✅ `checkDoubleDigits` should not contain unneeded console.log calls (1 ms) | ||
✅ should call new Promise() | ||
✅ `resolve()` should be called with a one argument (1 ms) | ||
✅ `reject()` should be called with a one argument | ||
✅ should be a function that takes a single argument (1 ms) | ||
✅ (9) should return a rejected promise with an Error object | ||
✅ (10) should return a promise that resolves to "This is a double digit number!" (1 ms) | ||
✅ (99) should return a promise that resolves to "This is a double digit number!" | ||
✅ (100) should return a rejected promise with an Error object | ||
|
||
Test Suites: 1 passed, 1 total | ||
Tests: 11 passed, 11 total | ||
Snapshots: 0 total | ||
Time: 0.86 s | ||
Ran all test suites matching /C:\\Users\\senyu\\hackyourfuture\\Assignments-cohort52\\.dist\\3-UsingAPIs\\Week1\\unit-tests\\ex2-checkDoubleDigits.test.js/i. | ||
No linting errors detected. | ||
No spelling errors detected. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
*** Unit Test Error Report *** | ||
|
||
PASS .dist/3-UsingAPIs/Week1/unit-tests/ex3-rollDie.test.js | ||
api-wk1-ex3-rollDie | ||
✅ should exist and be executable (2 ms) | ||
✅ should have all TODO comments removed | ||
✅ should call `new Promise()` | ||
✅ `resolve()` should be called with a one argument | ||
✅ `reject()` should be called with a one argument | ||
✅ should resolve when the die settles successfully (1 ms) | ||
✅ should reject with an Error when the die rolls off the table (1 ms) | ||
|
||
Test Suites: 1 passed, 1 total | ||
Tests: 7 passed, 7 total | ||
Snapshots: 0 total | ||
Time: 1.001 s | ||
Ran all test suites matching /C:\\Users\\senyu\\hackyourfuture\\Assignments-cohort52\\.dist\\3-UsingAPIs\\Week1\\unit-tests\\ex3-rollDie.test.js/i. | ||
No linting errors detected. | ||
No spelling errors detected. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
*** Unit Test Error Report *** | ||
|
||
PASS .dist/3-UsingAPIs/Week1/unit-tests/ex4-pokerDiceAll.test.js | ||
api-wk1-ex4-pokerDiceAll | ||
✅ should exist and be executable (2 ms) | ||
✅ should have all TODO comments removed (1 ms) | ||
✅ `rollDice` should not contain unneeded console.log calls | ||
✅ should use `dice.map()` | ||
✅ should use `Promise.all()` | ||
✅ should resolve when all dice settle successfully (11 ms) | ||
✅ should reject with an Error when a die rolls off the table (86 ms) | ||
|
||
Test Suites: 1 passed, 1 total | ||
Tests: 7 passed, 7 total | ||
Snapshots: 0 total | ||
Time: 0.721 s, estimated 1 s | ||
Ran all test suites matching /C:\\Users\\senyu\\hackyourfuture\\Assignments-cohort52\\.dist\\3-UsingAPIs\\Week1\\unit-tests\\ex4-pokerDiceAll.test.js/i. | ||
No linting errors detected. | ||
No spelling errors detected. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,17 @@ | ||
*** Unit Test Error Report *** | ||
|
||
PASS .dist/3-UsingAPIs/Week1/unit-tests/ex5-pokerDiceChain.test.js | ||
api-wk1-ex5-pokerDiceChain | ||
✅ should exist and be executable (1 ms) | ||
✅ should have all TODO comments removed | ||
✅ `rollDice` should not contain unneeded console.log calls | ||
✅ should resolve when all dice settle successfully (19 ms) | ||
✅ should reject with an Error when a die rolls off the table (43 ms) | ||
|
||
Test Suites: 1 passed, 1 total | ||
Tests: 5 passed, 5 total | ||
Snapshots: 0 total | ||
Time: 0.846 s | ||
Ran all test suites matching /C:\\Users\\senyu\\hackyourfuture\\Assignments-cohort52\\.dist\\3-UsingAPIs\\Week1\\unit-tests\\ex5-pokerDiceChain.test.js/i. | ||
No linting errors detected. | ||
No spelling errors detected. |
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!