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

Volodymyr-w1-UsingAPIs #51

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

Conversation

Volodymyr-coder
Copy link

No description provided.

@ddoyediran ddoyediran assigned ddoyediran and unassigned ddoyediran Feb 1, 2025
@ddoyediran ddoyediran requested review from ddoyediran and removed request for ddoyediran February 1, 2025 19:34
@ddoyediran ddoyediran self-assigned this Feb 1, 2025

callback(fullName);
}, 1000);
export const getAnonName = (firstName) => {

Choose a reason for hiding this comment

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

Great job with your solution to this first exercise (using Promise syntax instead of callbacks). The logic inside the setTimeout function is clear and follows the expected flow.

export const getAnonName = (firstName) => {
return new Promise((resolve, reject) => {
setTimeout(() => {
if (firstName) {

Choose a reason for hiding this comment

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

This looks great but a couple of things we can improve are:

  1. prevent redundant return statement:
    The return statement after resolve(fullName) is unnecessary because the resolve call ends the execution of the promise immediately.

  2. "Fail first" or "Fail Fast" approach:
    It is sometimes good to identify and report errors as early as possible in our code or development cycle. The benefit is that, it helps prevent bugs from propagating and becoming more difficult and costly to fix later on.

Original solution:

...
setTimeout(() => {
      if (firstName) {
        const fullName = `${firstName} Doe`;
        resolve(fullName);
        return;
      } else {
        reject(new Error("You didn't pass in a first name!"));
      }
    }, 1000);
...

Alternative: Here, we handle the case where the promise reject first. In addition, since, we use return statement in the if () {} statement, we don't have to add else {} block to our code.

...
setTimeout(() => {
  if (!firstName) {
    reject(new Error("You didn't pass in a first name!"));
    return;
  }
  resolve(`${firstName} Doe`);
}, 1000);
...

@@ -11,8 +11,14 @@ Complete the function called `checkDoubleDigits` such that:
"Expected a double digit number but got `number`", where `number` is the
number that was passed as an argument.
------------------------------------------------------------------------------*/
export function checkDoubleDigits(/* TODO add parameter(s) here */) {
// TODO complete this function
export function checkDoubleDigits(number) {

Choose a reason for hiding this comment

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

Well done, this is well written. Kudos.

return rollDie(1)
.then((value) => {
results.push(value);
return rollDie(2);
})
.then((value) => {

Choose a reason for hiding this comment

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

Great job, this works as expected.

But if you noticed, we are repeating these two codes from line 22 to 39:

 results.push(value);
 return rollDie(4);

We can prevent this with a Software engineering principle called "DRY Principle". "Don't Repeat Yourself" (DRY) is a software development principle that helps us to avoid duplicating code. If you aren't aware of it, you can Google Search.


### 3-UsingAPIs - Week1

| Exercise | Passed | Failed | ESLint |

Choose a reason for hiding this comment

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

@Volodymyr-coder Great job completing week 1 assignment. Overall, this is an impressive solution, and I can see that you put a lot of effort into it. Keep up the great work, and don't hesitate to ask if you have any questions or need further clarification on any of the points above. I will Approve it now.

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