Skip to content

Conversation

@Droid-An
Copy link

Learners, PR Template

Self checklist

  • I have committed my files one by one, on purpose, and for a reason
  • I have titled my PR with COHORT_NAME | FIRST_NAME LAST_NAME | REPO_NAME | WEEK
  • I have tested my changes
  • My changes follow the style guide
  • My changes meet the requirements of this task

Changelist

Completed Sprint 1 Coursework

Questions

No questions

@Droid-An Droid-An added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Feb 11, 2025
@cjyuan cjyuan added the Review in progress This review is currently being reviewed. This label will be replaced by "Reviewed" soon. label Feb 20, 2025
Copy link
Contributor

@cjyuan cjyuan left a comment

Choose a reason for hiding this comment

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

Looks good.
I left you some comments and suggestions.

Comment on lines +8 to +10
let initials = `${firstName.charAt(0)}${middleName.charAt(0)}${lastName.charAt(
0
)}`;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why broke the template string literal into three lines?

Copy link
Author

Choose a reason for hiding this comment

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

I think prettier did this. I also don't understand, why it broke it like this.

Comment on lines 20 to 21
const dir = filePath.slice(1, lastSlashIndex);
const ext = filePath.slice(filePath.indexOf(".") + 1);
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. The first / is also part of dir.
  2. Can you refine the expression for extracting the file extension so that it will work also for file path like "/Users/mitch/cyf.old/folder.bak/file.txt"?


/*
In 1 step program calculates "(maximum - minimum + 1)", which evaluates to 100. This is the total range of numbers between 1 and 100
In 2 step program multiples 100 by floating-point, pseudo-random number between 0 and 1.
Copy link
Contributor

Choose a reason for hiding this comment

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

The phrase "between 0 and 1" alone is not precise enough in program specification because
it does not state clearly whether 0 and 1 are included in the range.

You can look up Math.random() at MDN to find out exactly what value the function can return, ask ChatGPT how to concisely describe a range of numbers from 0 to 1 that include/exclude 0 and 1, and then use what you learn to describe the return value of Math.random().

let age = 33;
age = age + 1;

// we cannot change consonant data type, so we need to change const to let
Copy link
Contributor

Choose a reason for hiding this comment

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

What is consonant data type?
Also, age is not a data type.

console.log(`I was born in ${cityOfBirth}`);

/*
program couldn't find variable, because it was initialized after using, which is not correct
Copy link
Contributor

Choose a reason for hiding this comment

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

"declared" or "initialized"? They mean different things in programming.

Comment on lines 1 to 2
const HourClockTime12 = "20:53";
const hourClockTime24 = "08:53";
Copy link
Contributor

Choose a reason for hiding this comment

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

Have you also noticed the variable names do not quite match the values assigned to the variable?
Also, why started one of the variable names in uppercase and the other with a lowercase?

Copy link
Author

Choose a reason for hiding this comment

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

Thank you for pointing out, I haven't noticed it. Also I started one of the variable names in uppercase and the other with a lowercase because it was like this in the original code.

Comment on lines 15 to 18
// 3
// carPrice = Number(carPrice.replaceAll(",", ""));
// priceAfterOneYear = Number(priceAfterOneYear.replaceAll("," ""));
// console.log(`The percentage change is ${percentageChange}`);
Copy link
Contributor

Choose a reason for hiding this comment

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

foo(bar()) would be considered as two function calls in which bar() is called first and its return value is passed as a parameter to foo().

@cjyuan cjyuan added Reviewed Volunteer to add when completing a review with trainee action still to take. and removed Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. Review in progress This review is currently being reviewed. This label will be replaced by "Reviewed" soon. labels Feb 20, 2025
@Droid-An Droid-An added Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. and removed Reviewed Volunteer to add when completing a review with trainee action still to take. labels Feb 24, 2025
@Droid-An Droid-An requested a review from cjyuan February 24, 2025 20:04
Copy link
Contributor

@cjyuan cjyuan left a comment

Choose a reason for hiding this comment

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

Changes look great! Feel free to mark this PR as completed anytime.

/*
In 1 step program calculates "(maximum - minimum + 1)", which evaluates to 100. This is the total range of numbers between 1 and 100
In 2 step program multiples 100 by floating-point, pseudo-random number between 0 and 1.
In 2 step program multiples 100 by floating point, random number, that's greater than or equal to 0 and less than 1
Copy link
Contributor

Choose a reason for hiding this comment

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

One concise way to specify a range of values is to use the interval notation.
For example, we can say, Math.random() returns a number in [0, 1).

@Droid-An Droid-An added Reviewed Volunteer to add when completing a review with trainee action still to take. Complete Volunteer to add when work is complete and all review comments have been addressed. and removed Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. labels Feb 26, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Complete Volunteer to add when work is complete and all review comments have been addressed. Reviewed Volunteer to add when completing a review with trainee action still to take.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants