Skip to content

ILIA_BUBNOV-w1-UsingAPIs #34

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

MBreathe
Copy link

@MBreathe MBreathe commented Apr 7, 2025

No description provided.

@MBreathe MBreathe closed this Apr 7, 2025
@MBreathe MBreathe reopened this Apr 7, 2025
@durw4rd durw4rd self-assigned this Apr 9, 2025
Copy link

@durw4rd durw4rd left a comment

Choose a reason for hiding this comment

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

Good job, Ilia. You've met the requirements for all five exercises. I've added some comments where I thought you might be interested in seeing a different way of solving the assignment, but it's mostly for your information. Your work is good as it is.

Copy link

Choose a reason for hiding this comment

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

Regarding your comment about missing return statements in the original callback function, I didn't create this exercise, but I believe that that's the reason why the function erroneously returns both the Error and the Success callback if there are more than six rolls. The function execution is not stopped after the Error callback so it continues running till it reaches the number of originally scheduled rolls.

rollDie(num).then(resolve).catch(reject);
})
})
return Promise.all(promises);
Copy link

Choose a reason for hiding this comment

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

While this code works, it adds unnecessary redundancy and complexity since the rollDie function already returns a promise. You're creating a new Promise for each die, just to immediately pass through the result or error of another Promise (rollDie(num)), which already returns a Promise.

You could also leave out the extra promise and do

export function rollDice() {
  const dice = [1, 2, 3, 4, 5];
  const promises = dice.map(num => rollDie(num));
  return Promise.all(promises);
}

/*
I think that the rolls are happening inside the API and multiple Promises are running which causes the outputs to mix up when they go to
in the Event Queue and then end up in the Call Stack
*/
Copy link

Choose a reason for hiding this comment

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

I think you got it right, although you chose a rather technical language to explain it. It could also be summarized as:

  • Each rollDie(num) function is an independent async process.
  • Promise.all() does not cancel any promises; it just waits.
  • So even if one promise rejects, the rest keep going.

Copy link

Choose a reason for hiding this comment

The 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() handler for each die, capturing the die number in a closure.

const pushAndRoll = (dice) => (value) => {
  results.push(value);
  return rollDie(dice);
};

Then, you could do:

[...]
return rollDie(1)
    .then(pushAndRoll(2))
    .then(pushAndRoll(3))
    .then(pushAndRoll(4))
    .then(pushAndRoll(5))
    .then((value) => {
      results.push(value);
      return results;
    });

@MBreathe
Copy link
Author

Thanks a lot for the feedback, it is extremely valuable to me. Some things that you mentioned are very simple in the best way!

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

Successfully merging this pull request may close these issues.

2 participants