Skip to content

Manchester | 25-ITP-May | Jennifer Isidienu | Sprint 1 | Coursework #683

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

Conversation

JenniferIsidienu
Copy link

@JenniferIsidienu JenniferIsidienu commented Jul 25, 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 Region | Cohort | FirstName LastName | Sprint | Assignment Title
  • I have tested my changes
  • My changes follow the style guide
  • My changes meet the requirements of this task

Changelist

Briefly explain your PR.

Questions

Ask any questions you have for your reviewer.

@JenniferIsidienu JenniferIsidienu added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Jul 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.

  • Code is well written and explanation is clear. Well done.

  • Why not practice "committing files one by one, on purpose, and for a reason"?
    In VSCode, you can select which file to stage, and commit only the staged file.
    See: https://www.youtube.com/watch?v=z5jZ9lrSpqk&t=705 (At around 12:50 minute marker, the video shows how to stage a single file).

Comment on lines +39 to +42
// 5. const pence = paddedPenceNumberString.substring
// (paddedPenceNumberString.length - 2).padEnd(2, "0"):
// extracts the pence part of the string by taking the last two characters
// and ensuring it has exactly 2 characters by padding with zeros if necessary.
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

I think it's needed because the program may not work as intended if we deleted. Am I correct?

Copy link
Contributor

Choose a reason for hiding this comment

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

Why don't you try executing the script without using padEnd(2, '0'), and test it with different values of penceString to validate your expectation?

Copy link
Author

Choose a reason for hiding this comment

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

Everything still works fine. It seems that .padstart(3, "0") already ensures the string has at least 3 characters. So, when we extract the last two characters for the pence part, no extra padding is needed afterward.
Thanks a lot!

@cjyuan cjyuan 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. Reviewed Volunteer to add when completing a review with trainee action still to take. labels Jul 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants