-
Notifications
You must be signed in to change notification settings - Fork 26
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
Solution/kevinsalazar #13
base: master
Are you sure you want to change the base?
Conversation
const products = require('./products'); | ||
const prices = require('./prices'); |
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.
Imports should be at the beginning of the file
|
||
// You use Promise.all() here | ||
(async () => { |
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.
convert solution function into async function instead of doing this
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.
ok, the challenge allowed to modify solution function.
|
||
// You use Promise.allSettled() here | ||
try { | ||
[result.product, result.price] = await Promise.all([products(id), prices(id)]); |
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.
initialize other constants to get the results here
const [a, b] = await Promise.all()
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.
ok, that way it's more readable
// You use Promise.allSettled() here | ||
try { | ||
[result.product, result.price] = await Promise.all([products(id), prices(id)]); | ||
console.log( |
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.
console.log({id, a, b})
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.
ok, in this case printing as an object is better than formatting
|
||
// You generate your id value here | ||
const id = Number((Date.now()+'').slice(-2)); | ||
const result = {id}; |
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.
we can avoid doing it in this way. (See comments below)
console.log( | ||
` id: ${id}\ | ||
\n product: ${print(settled[0])}\ | ||
\n price: ${print(settled[1])}\n` | ||
); |
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.
print object
|
||
console.log('\nPromise.allSettled:\n') | ||
|
||
const settled = await Promise.allSettled([products(id), prices(id)]); |
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.
consider using more representative names
|
||
console.log('\nPromise.race:\n'); | ||
try { | ||
const value = await Promise.race([products(id), prices(id)]); |
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.
consider using more representative names
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.
my mistake
|
||
console.log('\nPromise.any:\n'); | ||
try { | ||
const value = await Promise.any([products(id), prices(id)]); |
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.
consider using more representative names
} catch (reason) { | ||
console.log(` error: ${reason.message}`); | ||
} | ||
})(); | ||
} | ||
|
||
solution() |
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.
should be async call
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.
What is Promise any and race for?
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.
Promise any and race are there just to compare visually their output with the Promise.all and Promise.allSettled ones, I did that just to take advantage of all outputs together.
const lastName = require('./lastnames'); | ||
const firstName = require('./firstnames'); |
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.
Imports should be at the beginning of the file
|
||
// You log the fullname, or error, here | ||
const fullName = {}; |
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.
We can define variables for firstname and lastname instead
// You call the firstname method | ||
.then(firstName => fullName.firstName = firstName) | ||
// You log the fullname, or error, here | ||
.then(_ => console.log(`${fullName.firstName} ${fullName.lastName}`)) |
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.
Here we can use the 2 constants created above
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.
ok, it is better and more readable that way
@@ -34,14 +34,46 @@ node solution.js name1 name2 name3 | |||
|
|||
function solution() { | |||
// YOUR SOLUTION GOES HERE | |||
const validateUser = require('./validate-user'); |
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.
imports should be at the beginning of the file
error ? | ||
console.log(error.message) | ||
: console.log(`\nid:${user.id}\nname: ${user.name}`); |
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.
validate existence of error using if condition
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.
Which considerations should we take using map?
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're right, my mistake in using map instead of other loop like forEach because map, in this case, is reducing performance.
} catch (reason) { | ||
console.log(` error: ${reason.message}`); | ||
} | ||
})(); | ||
} | ||
|
||
solution() |
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.
What is Promise any and race for?
new Promise (resolve => { | ||
validateUser(name, (error, user) => { | ||
error ? | ||
resolve(failure.push(error.message)) | ||
: resolve(success.push(user)); | ||
}) | ||
}) |
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.
Use promisify method
|
||
function solution() { | ||
// YOUR SOLUTION GOES HERE | ||
let validateUser = require('./validate-user'); |
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.
const instead of let and it should be at the beginning of the file
No description provided.