Cape Town | 26-ITP-Sept | Leigh Ross | Sprint 1 | Exhaustively test and fix formatAs12HourClock

[leigh-ross]

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

• Fixed bugs in timeConverter.js
• Added new tests in timeConverter.test.js

Questions

I had already completed this task in the previous module so I just did the same code. I hope this is alright.

[cjyuan]

Part of the challenge in this exercise is to design a comprehensive test suite that can detect potential errors not only in your own function implementation but also in future modifications made by others.

What additional test data should be included to make the test suite more comprehensive?

[leigh-ross]

I have added more test cases to really catch any further bugs. It was a bit difficult so, I used a bit of help from AI especially for that last 24 hour sweep test.
I expanded my tests with zero-padding tests for inputs such as "08:30" and "10:05".
I also added boundary tests around "noon" and "midnight".
The 24 hour sweep tests should cover everything else and ensure that any bugs will be caught.

[cjyuan]

That's a thorough test.

Did you pick up any thought process or logic from the AI for deriving the test data? This way, you'd have a better idea of what to test or check when implementing similar functions in the future.

[cjyuan]

Code works.

Could you use AI to explore alternatives to simplify the code on lines 4-22 into 3-4 lines of code?

[leigh-ross]

This is what AI gave me:
Option 1: Ternary + modulo (3 lines)
const hours12 = hours % 12 || 12;
const timePeriod = hours < 12 ? "am" : "pm";
return ${String(hours12).padStart(2, "0")}:${minutes} ${timePeriod};

• hours % 12 gives 0 for midnight/noon, so || 12 converts that to 12
• hours < 12 correctly identifies am (0-11); everything else is pm

Option 2: Single expression (1 line)
return ${String(hours % 12 || 12).padStart(2, "0")}:${minutes} ${hours < 12 ? "am" : "pm"};

Option 3: With toLocaleTimeString (fully different approach)
const [h, m] = time.split(":").map(Number);
return new Date(2000, 0, 1, h, m).toLocaleTimeString("en-US", {
hour: "2-digit", minute: "2-digit", hour12: true
}).toLowerCase();

Out of those 3 options, I like option 1 the best:

• It is more concise that my current code but there is still enough info from the variables for me to understand at a glance.
• The hours 12 handling is also much shorter without using if - else blocks
• I would still prefer to declare a function just to ensure that my program is not full of global variables.

Would you like me to change my code so that option 1 is implemented correctly?

[cjyuan]

The code submitted to a PR should be one's best work.
So I think it is a good practice to apply what you think is an improved solution.

Also, by implementing them, you could strengthen you understanding.

I agree option 1 is better than then other two.

---

I would still prefer to declare a function just to ensure that my program is not full of global variables.

What do you mean? Why would the change involve any global variables?

[cjyuan]

Changes look good.