Skip to content

Sheffield | May-2025 | Mayowa Fadare | Structuring and Testing Data Sprint-1 #608

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 13 commits into
base: main
Choose a base branch
from

Conversation

mayowa0-7
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 REGION | COHORT_NAME | FIRST_NAME LAST_NAME | PROJ_NAME
  • I have tested my changes
  • My changes follow the style guide
  • My changes meet the requirements of this task

Changelist

i worked on the template in Structuring and Testing Data Sprint-1, by reading and answering the questions where neccessary.

Questions

Ask any questions you have for your reviewer.

@mayowa0-7 mayowa0-7 added the Needs Review Participant to add when requesting review label Jun 25, 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.

You have some modified files in "Sprint-3" folders. Can you revert the changes made to the files in that folder to keep this branch clean?

Comment on lines 8 to 9
let initials = `firstname(0)+middlename(0)+lastname(0)`;
console.log(initials);
Copy link
Contributor

@cjyuan cjyuan Jun 27, 2025

Choose a reason for hiding this comment

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

Have you tested these code to ensure it works as expected?

Copy link
Author

@mayowa0-7 mayowa0-7 Jun 28, 2025

Choose a reason for hiding this comment

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

yeah i tested it now and it was not working, but i have fixed it now.

@@ -7,3 +7,8 @@ const num = Math.floor(Math.random() * (maximum - minimum + 1)) + minimum;
// Try breaking down the expression and using documentation to explain what it means
// It will help to think about the order in which expressions are evaluated
// Try logging the value of num and running the program several times to build an idea of what the program is doing
Math.random() means it can generates a random decimal 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.

Phrases like "number in range from 0 to 1" or "between 0 and 1" are not precise enough in a program specification, because they do not clearly state whether the endpoints 0 and 1 are included.

We can use the concise and precise interval notation to describe a range of values.
For example, we can say, "Math.random() returns a random number in the interval [0, 1)"

Copy link
Author

Choose a reason for hiding this comment

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

thanks for the comment, it now corrected.

@@ -12,11 +12,12 @@ console.log(`The percentage change is ${percentageChange}`);
// Read the code and then answer the questions below

// a) How many function calls are there in this file? Write down all the lines where a function call is made

There are 3 function calls on this file: in line 4, 5 and 10.
Copy link
Contributor

Choose a reason for hiding this comment

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

There are more than 3 function calls.

Copy link
Author

Choose a reason for hiding this comment

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

yes you correct cj, there are actually 5 fucntions call

// e) Describe what the expression Number(carPrice.replaceAll(",","")) is doing - what is the purpose of this expression?
To replace all the comma in the string price to just numerical values (10,000 to 10000)
Copy link
Contributor

Choose a reason for hiding this comment

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

Could also use the say "remove the comma in ....".

The whole sentence is not quite grammatically correct. Can you use an AI tool to fix the grammar mistake?

Copy link
Author

Choose a reason for hiding this comment

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

fixed now

Comment on lines 29 to 36
const penceStringWithoutTrailingP = penceString.substring(0, penceString.length - 1); removes the trailing p from the penceString, so the value will be "399".

const paddedPenceNumberString = penceStringWithoutTrailingP.padStart(3, "0"); pads the numeric string with zeros until it 3 digits character Long. value will be "399" and also ensuring consistency of 3 digits value.

const pounds = paddedPenceNumberString.substring(0, paddedPenceNumberString.length - 2); extracts the pounds part from the padded string by taking all but the last two digits.Here, it will be "3".

const pence = paddedPenceNumberString.substring(paddedPenceNumberString.length - 2).padEnd(2, "0"); extracts the last two characters as the pence portion and ensures it is always two digits.
console.log(`£${pounds}.${pence}`); formats and prints the final price in pounds and pence format, hence it will output "£3.99"
Copy link
Contributor

Choose a reason for hiding this comment

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

  • Why not write the description as comments so that you can still execute the code in the script?

  • Could we expect this program to work as intended for any valid penceString if we deleted .padEnd(2, "0") from the code? In other words, do we really need .padEnd(2, "0") in this script?

Copy link
Author

Choose a reason for hiding this comment

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

No we don't actually need it, the code still worked perfectly without the .padEnd (2, "0").

@cjyuan cjyuan added Reviewed Volunteer to add when completing a review and removed Needs Review Participant to add when requesting review labels Jun 27, 2025
@mayowa0-7 mayowa0-7 added Needs Review Participant to add when requesting review and removed Reviewed Volunteer to add when completing a review labels Jun 28, 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.

All the other changes look good.

Can you rephrase the description in Sprint-1/3-mandatory-interpret/1-percentage-change.js?

// e) Describe what the expression Number(carPrice.replaceAll(",","")) is doing - what is the purpose of this expression?
// To remove the comma in the string price to just numerical values (10,000 to 10000)
Copy link
Contributor

Choose a reason for hiding this comment

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

The whole sentence is still not quite grammatically correct. Can you use an AI tool to fix the grammar mistake?

@cjyuan cjyuan added Reviewed Volunteer to add when completing a review and removed Needs Review Participant to add when requesting review labels Jun 28, 2025
@mayowa0-7 mayowa0-7 added Needs Review Participant to add when requesting review and removed Reviewed Volunteer to add when completing a review labels Jul 2, 2025
@cjyuan
Copy link
Contributor

cjyuan commented Jul 3, 2025

Change look good! Well done.

@cjyuan cjyuan added Complete Volunteer to add when work is complete and review comments have been addressed and removed Needs Review Participant to add when requesting review labels Jul 3, 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 review comments have been addressed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants