-
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/RenatoMonroy #22
base: master
Are you sure you want to change the base?
Conversation
|
||
// iterate the names array and validate them with the method | ||
console.log('Success'); | ||
for (const name of 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.
just to add readability, always separate blocks of code like loops, conditionals, etc using a line break at the start of that block
function callback(...params) { | ||
if (params.length > 1) | ||
console.log(`\nid: ${params[1].id}\nname: ${params[1].name}\n`); | ||
else { | ||
setTimeout(() => { | ||
console.log(params[0].message); | ||
}, 400); | ||
} | ||
} |
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 must keep the standards, if we know that the first argument is the error, and the second argument is the data, we must name these parameters in this way, so it will help other devs to understand your code easily
validateAsync(name, (...params) => { | ||
if (params.length > 1) console.log(`\nid: ${params[1].id}\nname: ${params[1].name}\n`) | ||
else throw new Error(params[0].message); | ||
}).catch(error => setTimeout(() => console.error(error.message), 350)); | ||
} | ||
setTimeout(() => console.log('Failure\n'), 350); | ||
} |
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.
since we've promised the validateName
function, we could use promises or async/await on the validateAsync
instead of a callback
process.stdout.write(`${lname}, `); | ||
|
||
// Firstname callback | ||
firstnames(lname).then(fname => { | ||
process.stdout.write(`${fname}\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.
since we are returning both lname
and name
together, we could just use a process.stdout.write
in the second then
method
function randomId() { | ||
const desire = [1, -1]; | ||
const choice = Math.floor(Math.random() * desire.length); | ||
return (Math.floor(Math.random() * 2)) ? |
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.
the same recommendation about readability for a return
console.log({ id, product: promisesFulfilled[1], price: promisesFulfilled[0] }); | ||
|
||
} catch (error) { | ||
/* NOTE : I've been trying to do proper error handling with all the promises but I had one struggle |
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.
Keep in mind the difference between Promise.all and Promise.allSettled is that in the first an error could be thrown any moment so we should use a try/catch. But in the second case, it stores the results on each promise, so the error is stored too. It seems to me that line 64 is enough
Completed the challenge with all extra steps