Cape Town | sep-2026-ITP | Matthaus Bause | Sprint 3 | Dead Code by chefmatthaus-hub · Pull Request #1755 · CodeYourFuture/Module-Structuring-and-Testing-Data

chefmatthaus-hub · 2026-10-25T15:39:40Z

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

CYF-1006

… midnight and noon

…or single-digit morning hours

mayowaolokun

Great work, you clearly understand how to spot unused and unreachable code, and your formatAs12HourClock fix handles the edge cases correctly, including midnight, noon and times with minutes. Planning your edge cases before writing the tests was a really good approach.

I have left a few comments. Mainly on lines 45–47 of timeConverter.test.js, as it may not be checking what you expect. The rest are tidy-ups around consistency, duplicate test names and file formatting.

You’re very close. Once those are addressed, this will be in great shape.

mayowaolokun · 2026-10-25T23:34:32Z

-// The sayHello function should continue to work for any reasonable input it's given.
-
-let testName = "Jerry";
+let testName = "Aman";


Nice work spotting the unreachable console.log and the unused greetingStr, and setting testName correctly from the start is a neat simplification. Since testName is no longer reassigned anywhere, have another look at how it’s declared and think about which keyword best signals that its value won’t change.

mayowaolokun · 2026-10-25T23:43:51Z

@@ -1,17 +1,9 @@
-// Remove the unused code that does not contribute to the final console log
-// The countAndCapitalisePets function should continue to work for any reasonable input it's given, and you shouldn't modify the pets variable.
-


Well done for removing capitalisedPets and logPets, and for keeping petsStartingWithH, which the final output depends on. Line 3 hasn’t changed, but because it has moved, Git shows it as deleted and re-added, so reviewers have to check that it wasn’t altered. When removing code, try to leave the remaining lines where they were so the diff shows only your intended changes. Checking your diff before pushing is a good way to catch this.

mayowaolokun · 2026-10-25T23:47:14Z

 }

-export {formatAs12HourClock};
+export {formatAs12HourClock};


The icon here means the file no longer ends with a newline, which is why this unchanged line shows as modified. Most editors and tools expect files to end with a newline, so it’s worth checking your editor settings (Prettier on save should handle this for you).

mayowaolokun · 2026-10-25T23:51:34Z

+  if (hours === 12) {
+    return `12:${minutes} pm`;
+  }

  if (hours > 12) {
-    return `${hours - 12}:00 pm`;
+    const formattedHours = String(hours - 12).padStart(2, '0');
+    return `${formattedHours}:${minutes} pm`;
  }
-  return `${time} am`;
+
+  const formattedHours = String(hours).padStart(2, '0');
+  return `${formattedHours}:${minutes} am`;


Great work handling the edge cases.
Since this task is about edge cases, make sure your tests cover them too, such as "00:00", "12:00", and a time with minutes like "13:45", so you can be confident that each branch works.

mayowaolokun · 2026-10-25T23:54:51Z

+
+  const [hoursStr, minutesStr] = time.split(':');
+  const hours = Number(hoursStr);
+  const minutes = minutesStr;


This line creates a second variable holding exactly the same value as minutesStr. Is there a way you could avoid the extra variable while keeping the code readable?

mayowaolokun · 2026-10-25T23:59:11Z

+test("correctly convert single-digit morning hours if supported", () => {
+    assert(formatAs12HourClock("08:30"), "08:30am");
+});


This test uses assert(...) rather than assert.equal(...) like the others. Have a look at what the second argument to assert() actually does, and whether this test could ever fail. It’s also worth checking that the input matches the test name (is "08:30" a single-digit hour?) and that the expected output is formatted the same way as your other tests.

mayowaolokun · 2026-10-26T00:00:04Z

+    assert.equal(formatAs12HourClock("08:30"), "08:30 am");
+});
+
+test("can correctly convert morning time minutes", () => {


This test has the same name as the one on line 17. Unique names make it much easier to tell which test failed when something breaks.

mayowaolokun · 2026-10-26T00:01:56Z

+test("can successfully convert midnight time", () =>
+    assert.equal(formatAs12HourClock("24:00"), "12:00 am"));


This test is written differently from the others (no curly braces or semicolons). Keeping a consistent style makes the file easier to read.

mayowaolokun · 2026-10-26T00:03:27Z

+
+Correctly convert early afternoon hours 
+Input | Expected
+13:00   1:00 pm


The comment here expects 1:00 pm, but the test on line 42 expects 01:00 pm. Which format does your function produce? It’s worth making the comment and test agree so they don’t confuse.

mayowaolokun · 2026-10-26T00:08:35Z

+Correctly convert single-digit morning hours if supported
+Input | Expected
+08:30   8:30 am
+*/


Check the comment on line R24 as it applies here.

chefmatthaus-hub added 6 commits October 5, 2026 16:38

Refactor time conversion logic and enhance test coverage for edge cases

2ec464d

Refactor time conversion logic and add tests for edge cases including…

0795731

… midnight and noon

Enhance test coverage for single-digit morning hours in time conversion

7753fc4

Fix formatting issues in time conversion and update test assertions f…

f357b30

…or single-digit morning hours

Refactor sayHello function and update testName variable for clarity

e2e47e3

Remove unused code and streamline pet filtering logic

ca27335

chefmatthaus-hub added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Oct 25, 2026

mayowaolokun added Review in progress This review is currently being reviewed. This label will be replaced by "Reviewed" soon. and removed Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. labels Oct 25, 2026

mayowaolokun reviewed Oct 26, 2026

View reviewed changes

mayowaolokun added Reviewed Volunteer to add when completing a review with trainee action still to take. and removed Review in progress This review is currently being reviewed. This label will be replaced by "Reviewed" soon. labels Oct 26, 2026

Navigation Menu

Repository navigation

Cape Town | sep-2026-ITP | Matthaus Bause | Sprint 3 | Dead Code - #1755

Cape Town | sep-2026-ITP | Matthaus Bause | Sprint 3 | Dead Code #1755
chefmatthaus-hub wants to merge 6 commits into
CodeYourFuture:mainfrom
chefmatthaus-hub:coursework/sprint-3-dead-code

chefmatthaus-hub commented Oct 25, 2026

Uh oh!

mayowaolokun left a comment

Uh oh!

mayowaolokun Oct 25, 2026

Uh oh!

mayowaolokun Oct 25, 2026

Uh oh!

mayowaolokun Oct 25, 2026

Uh oh!

mayowaolokun Oct 25, 2026

Uh oh!

mayowaolokun Oct 25, 2026

Uh oh!

mayowaolokun Oct 25, 2026

Uh oh!

mayowaolokun Oct 26, 2026

Uh oh!

mayowaolokun Oct 26, 2026

Uh oh!

mayowaolokun Oct 26, 2026

Uh oh!

mayowaolokun Oct 26, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		@@ -1,17 +1,9 @@
		// Remove the unused code that does not contribute to the final console log
		// The countAndCapitalisePets function should continue to work for any reasonable input it's given, and you shouldn't modify the pets variable.

		test("can successfully convert midnight time", () =>
		assert.equal(formatAs12HourClock("24:00"), "12:00 am"));

Conversation

chefmatthaus-hub commented Oct 25, 2026

Uh oh!

mayowaolokun 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!

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!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants