Skip to content
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

✨ Async Nerdery Challenges Solutions #32

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

marvnramos
Copy link

No description provided.

@@ -32,16 +32,75 @@ node solution.js name1 name2 name3
** give a look to node.js util.promisify, avoid to alter the validate-user.file **
*/

function solution() {
// YOUR SOLUTION GOES HERE
const { Console } = require('node:console');

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove unused import


// iterate the names array and validate them with the method
const promises = names.map(name => {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are not supposed to use promises in here. You should be using only callbacks to resolve this first exercise. Please update it accordingly


function solution(names = []) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function is not being used, and it should

// you get your 5 names here
if (names.length === 0) names = NAMES;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even if there is only one inner statement, always wrap your for loop and conditional statements in brackets, so it's more readable

// you get your 5 names here
if (names.length === 0) names = NAMES;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could also take advantage of the Javascript's truthiness. This could also be

if (!names.length)

However, that's just a comment in case you didn't know. I, personally, prefer to be really explicit most of the time, just like you did

firstname = name
})
.catch((error) => console.error(error))
.finally(() => console.log(`fullname: ${firstname} ${lastname}`))

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.finally() is usually used for clean up or actions that should happen either if the promise resolves or rejects. This statement should be in the then part of the code, as right now it prints fullname: undefined undefined when we get an error

function getRandomId() {
const random = Math.random();
if (random < 0.5) return '❌';
const id = Math.floor(Math.random() * 100)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will only generate values from 0-99, not up to 100 as requested

const prices = require('./prices');

function getId() {
return Number(String(Date.now()).slice(-2));

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does work, but is really uncommon to use constructors for primitive types. You should have done something like

parseInt(Date.now().toString().slice(-2))

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could also use an arrow function for simplicity

const calculateRandomId = () => parseInt(Date.now().toString().slice(-2))

// You use Promise.all() here
// You use Promise.allSettled() here
// Log the results, or errors, here
const [product, price] = await Promise.all([products(id), prices(id)]);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could have wrapped this and the Promise.all related code in a try catch, so the rest of the code works even if this one fails

}
} catch (error) {
console.log(error)
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a comment elaborating on your conclusion about Promise.race vs Promise.any

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants