Manchester | ITP-Jan-26 | Ofonime Edak| Sprint 2 |Structuring and testing Data#1097
Open
Ofonimeedak wants to merge 8 commits intoCodeYourFuture:mainfrom
Open
Manchester | ITP-Jan-26 | Ofonime Edak| Sprint 2 |Structuring and testing Data#1097Ofonimeedak wants to merge 8 commits intoCodeYourFuture:mainfrom
Ofonimeedak wants to merge 8 commits intoCodeYourFuture:mainfrom
Conversation
This comment has been minimized.
This comment has been minimized.
Ofonimeedak
added
📅 Sprint 2
This comment has been minimized.
This comment has been minimized.
github-actions
bot
removed
the
Needs Review
This comment has been minimized.
This comment has been minimized.
2 similar comments
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Ofonimeedak
added
the
Needs Review
This comment has been minimized.
This comment has been minimized.
github-actions
bot
removed
the
Needs Review
This comment has been minimized.
This comment has been minimized.
Ofonimeedak
added
the
Needs Review
This comment has been minimized.
This comment has been minimized.
github-actions
bot
removed
the
Needs Review
This comment has been minimized.
This comment has been minimized.
Ofonimeedak
added
the
Needs Review
This comment has been minimized.
This comment has been minimized.
github-actions
bot
removed
the
Needs Review
Ofonimeedak
changed the title
Ofonimeedak
added
the
Needs Review
cjyuan
added
Review in progress
cjyuan
reviewed
Mar 7, 2026
cjyuan
added
Reviewed
Ofonimeedak
added
the
Needs Review
cjyuan
reviewed
Mar 8, 2026
Contributor
cjyuan
left a comment
cjyuan
left a comment
There was a problem hiding this comment.
Please note that in CYF courses, the recommended way to inform the reviewer of your changes is to do both of the following:
- Reply to their feedback.
- In the responses, clarify how each piece of feedback was addressed to demonstrate that you've carefully reviewed the suggestions.
- You may find the suggestions in this PR Guide useful.
- Your response may trigger a notification (depending on the reviewer's settings), helping ensure they’re aware of the updates you’ve made.
- In the responses, clarify how each piece of feedback was addressed to demonstrate that you've carefully reviewed the suggestions.
- Replace the "Reviewed" label by a "Needs review" label (which you have done -- great!)
- Without this label, the reviewer would not know if your changes is ready to be reviewed.
cjyuan
removed
the
Needs Review
Author
|
Thank you very much for your review. I can confirm now that I can spot the bugs after running more test cases. I will commit and resubmit ASAP |
Author
|
Hi CJ, I have made the necessary change in my new commit. Please have a look. Thank you |
Ofonimeedak
added
the
Needs Review
cjyuan
reviewed
Mar 8, 2026
cjyuan
removed
the
Needs Review
Ofonimeedak
added
the
Needs Review
Author
|
Added a padStart function to hours between 1 and 9 to ensure output consistency |
cjyuan
reviewed
Mar 12, 2026
Contributor
cjyuan
left a comment
cjyuan
left a comment
There was a problem hiding this comment.
Code is working well. Good job.
Comment on lines
+6
to
+10
| const hours = time.slice(0, 2); | ||
| const minutes = time.slice(3, 5); | ||
|
|
||
| if (Number(hours) > 12) { | ||
| let timeHour = Number(hours) - 12; |
Contributor
There was a problem hiding this comment.
Why not convert hours to a number on line 6 once (instead of repeatedly convert it)?
If you need also hours as a string, you can use a separate variable to store hours as a number.
|
|
||
| if (Number(hours) > 12) { | ||
| let timeHour = Number(hours) - 12; | ||
| if (timeHour.toString().length !== 2) { |
Contributor
There was a problem hiding this comment.
Could also check timeHour < 10.
| if (Number(hours) > 12) { | ||
| let timeHour = Number(hours) - 12; | ||
| if (timeHour.toString().length !== 2) { | ||
| timeHour = timeHour.toString().padStart(2, "0"); |
Contributor
There was a problem hiding this comment.
timeHour was initially declared to stored a converted hour as a number, so it is a best practice not to reuse it to store a different kind of value. A better approach is to introduce a separate variable.
cjyuan
added
Complete
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Self checklist
Changelist
I have followed the instructions and responded to the question to the best of my understanding
This PR covers debugging of function errors, testing, and structuring data