London | 26-ITP-Sep | Sakiya Mayow | Sprint 1 | Structuring and Testing Data by zakiaao-tech · Pull Request #1639 · CodeYourFuture/Module-Structuring-and-Testing-Data

zakiaao-tech · 2026-10-03T15:23:40Z

Learners, PR Template

Self checklist

I have titled my PR with Region | Cohort | FirstName LastName | Sprint | Assignment Title
My changes meet the requirements of the task
I have tested my changes
My changes follow the style guide

Task code

CYF-1197

Changelist

In this task I added an if statement because when testing the code I noticed that for any hours < 10 is a single digit number but we need double digits and added 0 where it was supposed to go. I then tested a few edge cases and fixed the function according to the mistakes and failed tests.

cjyuan

Your test misses several cases.

You should update your test first before revising your function implementation.

abdishakoor-dev

Writing the tests first and then adding minutes to the function, as your last two commits show, was the right order. 23:59 and 12:01 both come out right now.

There's one bug left in the function, and a few tests to add that will show it to you. Here are the steps:

Add a test for each of these times: "11:59", "00:00", "00:34", "12:00". Write each test in the same shape as yours. The first "..." is the time going in. The second "..." is what your function should give back, as a 12-hour clock would show it. Decide that yourself before you run anything, for example: what does a 12-hour clock show at 00:34?
```
test("describe the time you are checking", function () {
  assert.equal(formatAs12HourClock("..."), "...");
});
```
Run the tests. Right click the format-clock-edge-cases folder in VS Code, choose Open in Integrated Terminal, and run node --test. The 11:59 test should fail and show you what your function really returns: 11:59: 59 am. That's good. It means the test has found a bug, and every time from 10:00 to 11:59 has it.
Fix the bug. It's on timeConverter.js line 24 (see my comment there). Run node --test again until every test passes.
Sort out the four tests on lines 13 to 27. See my comment on line 13.

When all of that is pushed and every test passes, it should be ready to mark Complete. Add the Needs Review label again once you've pushed.

abdishakoor-dev · 2026-10-06T12:28:10Z

+    return `0${hours}:${minutes} am`;
+  }
+
+  return `${time}: ${minutes} am`;


time holds the whole input, for example "10:30". So ${time} already includes the minutes, and : ${minutes} then adds them a second time, with a space in front. That's how you get 10:30: 30 am.

For 10:00 to 11:59 the hours already have two digits, so nothing needs changing about them. What does ${time} on its own give you, and is that all you need before am?

abdishakoor-dev · 2026-10-06T12:28:10Z

+  assert.equal(formatAs12HourClock("23:59"), "11:59 pm");
+});
+
+test("converts 12:01 to 12:01 ", function () {


Bugs often hide at the exact point where something changes. 12:01 is the first minute after noon, so the last minute before noon, 11:59, is just as important. Your function gets 11:59 wrong at the moment, and a test for it would have shown you. Midnight (00:00) and noon (12:00) are the other two points where the clock changes, so they need a test each too.

abdishakoor-dev · 2026-10-06T12:28:10Z

+  assert.equal(formatAs12HourClock("08:00"), "08:00 am");
+});
+
+test("correctly convert time after 12:00 ", function () {


These four tests all have the same name, "correctly convert time after 12:00". If one of them failed, the output wouldn't tell you which time broke. They also all check a whole hour between 2pm and 9pm, so they all test the same thing.

Keep one of them, and give it a name that says the time it checks, for example "converts 14:00 to 02:00 pm". Delete the other three. Your new tests from step 1 of my overall comment test far more.

cjyuan

Could you also respond to all the reviewers' comments in addition to making change?

Suggestion: Use AI to explore "how to respond to comments in a PR".

cjyuan · 2026-10-12T11:11:29Z

+    if (time < 10) {
+      return `0${time}:${minutes} pm`;
+    }
+
+    return `${hours - 12}:${minutes} pm`;


Could you try simplifying the code on lines 16-20 into one line of code by taking advantage of a String's built-in method (function) ?

cjyuan · 2026-10-12T15:51:49Z

+test("converts 23:59 to 11:59 ", function () {
+  assert.equal(formatAs12HourClock("23:59"), "11:59 pm");
+});
+
+test("converts 12:01 to 12:01 ", function () {
+  assert.equal(formatAs12HourClock("12:01"), "12:01 pm");
+});
+
+test("converts 23:59 to 11:59 ", function () {
+  assert.equal(formatAs12HourClock("23:59"), "11:59 pm");
+});


Duplicate test.

Thank you for the feedback. I've made changes, added the requested tests, and run node --test to check result.

cjyuan · 2026-10-12T16:04:55Z

 function formatAs12HourClock(time) {
-
  const hours = Number(time.slice(0, 2));
+  const minutes = time.slice(3, 5);


Note: The .slice() method supports negative indices, which count positions from the end of the string.

For example, str.slice(-3) returns the substring containing last three characters from str.

cjyuan · 2026-10-12T16:11:21Z


  if (hours > 12) {
-    return `${hours - 12}:00 pm`;
+    const time = hours - 12;


time is also the name of the parameter.

Could you explain why using the same name for a local variable and a parameter is considered bad practice?

Time is already the parameter in my function, declaring it again in the same scope can cause naming conflict.

cjyuan · 2026-10-27T00:34:51Z

It seems some of the inline comments were not yet addressed. Could you address all the inline comments in addition to making changes?

Suggestion: Use AI to explore "How to respond to inline comments in a PR"

zakiaao-tech added 2 commits October 3, 2026 16:15

if statement added

b80ce87

edge cases

4092ead

zakiaao-tech added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Oct 3, 2026

cjyuan reviewed Oct 4, 2026

View reviewed changes

Comment thread format-clock-edge-cases/timeConverter.test.js

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. labels Oct 4, 2026

zakiaao-tech added 2 commits October 5, 2026 23:47

added boundary cases

ce94896

added minutes to function

379bec4

zakiaao-tech added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Oct 5, 2026

abdishakoor-dev reviewed Oct 6, 2026

View reviewed changes

abdishakoor-dev removed the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Oct 6, 2026

zakiaao-tech added 2 commits October 9, 2026 17:44

added and removed a few tests

68d0b4c

ordered and structured my function

525f9fc

zakiaao-tech added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Oct 9, 2026

cjyuan reviewed Oct 12, 2026

View reviewed changes

cjyuan removed the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Oct 12, 2026

zakiaao-tech added 2 commits October 26, 2026 23:25

added more tests

701e223

fixed line 27

373fee6

zakiaao-tech added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Oct 26, 2026

cjyuan removed the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Oct 27, 2026

Conversation

zakiaao-tech commented Oct 3, 2026

Learners, PR Template

Task code

Changelist

Uh oh!

cjyuan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

abdishakoor-dev left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cjyuan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cjyuan commented Oct 27, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants