Skip to content

Conversation

@Fradoka
Copy link

@Fradoka Fradoka commented Feb 7, 2025

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 tasks for Sprint 1 of module Structuring and storing data

Questions

Ask any questions you have for your reviewer.

@Fradoka Fradoka added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Feb 7, 2025
Copy link

@GergelyKI GergelyKI left a comment

Choose a reason for hiding this comment

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

It looks good, I left a few comment here and there.

// This should produce the string "CKJ", but you must not write the characters C, K, or J in the code of your solution.

let initials = ``;
let initials = `${firstName.charAt(0)+middleName.charAt(0)+lastName.charAt(0)} `;

Choose a reason for hiding this comment

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

Here, you concatenate strings values with the + operator. This result in a string already. There's no need to use the template literal: ${} .

Advanced tip:
Concatenating strings with the + operator is considered slow in general. This is what the template literals can help with. So if you do it like this, probably it could be faster:

${firstName.charAt(0)} ${middleName.charAt(0)} ${lastName.charAt(0)}

// the math.random() generates a random number from 0 to 1
// the Math.random() * (maximum - minimum + 1) scales the random number to a range of (maximum - minimum + 1)
// this helps to cover all possible values from minimum to maximum
// the Math.floor(...) rounds up the result to make sure we get an integer value

Choose a reason for hiding this comment

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

Is this rounding up for sure? Why would the name be floor?

hint: there's another function, Math.ceil(...)

@@ -1,6 +1,11 @@
const cardNumber = 4533787178994213;
const cardNumber = "4533787178994213";

Choose a reason for hiding this comment

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

This works, however you have change the data type from number to string?

How could you keep it a number?

Tip: the last 4 digits mean the remainder if you divide by 10 000.

@Fradoka
Copy link
Author

Fradoka commented Feb 11, 2025

@GergelyKI i reviewed and addressed feedback. could you please review

@Fradoka
Copy link
Author

Fradoka commented Feb 13, 2025

@halilibrahimcelik could you please review my code?

@halilibrahimcelik halilibrahimcelik 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. labels Feb 13, 2025
@Fradoka Fradoka added the Complete Volunteer to add when work is complete and all review comments have been addressed. label Feb 14, 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.

4 participants