Cape Town | 26-ITP-Jan | Pretty Taruvinga | Sprint 3 | Practice TDD#1133
Cape Town | 26-ITP-Jan | Pretty Taruvinga | Sprint 3 | Practice TDD#1133Pretty548 wants to merge 12 commits intoCodeYourFuture:mainfrom
Conversation
… including edge cases
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.
Pretty548
added
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.
2 similar comments
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…om> reverted to main
Pretty548
added
the
Needs Review
This comment has been minimized.
This comment has been minimized.
github-actions
bot
removed
the
Needs Review
Pretty548
added
the
Needs Review
cjyuan
left a comment
cjyuan
left a comment
There was a problem hiding this comment.
Functions look good.
Tests could use some improvement.
Also, can you remove any unused code and unnecessary comments from the files to keep them cleaner? They make the code less readable.
| test("should be case sensitive", () => { | ||
| const str = "Hello World"; | ||
| const char = "H"; | ||
| const count = countChar(str, char); | ||
| expect(count).toEqual(1); | ||
| }); |
There was a problem hiding this comment.
Nice touch to to show match is case sensitive.
Could also consider a case to show that the function is expected to work also for non-alphabets.
| return "1st"; | ||
| const suffixes = ["th", "st", "nd", "rd"]; | ||
| const v = num % 100; | ||
| return num + (suffixes[(v - 20) % 10] || suffixes[v] || suffixes[0]); |
There was a problem hiding this comment.
Fancy!
To check your understanding:
Suppose v is 97, then both
97 % 10 = 7
(97-20) % 10 = 77 % 10 =7
Why not express line 4 as return num + (suffixes[v % 10] || suffixes[v] || suffixes[0]);?
| // Case 4: All other numbers | ||
| // When the number does not end with 1, 2, or 3 (or ends with 11, 12, or 13), | ||
| // Then the function should return a string by appending "th" to the number. | ||
| test("should append 'th' for all other numbers", () => { |
There was a problem hiding this comment.
When a test fails with the message "... all other numbers", it may be unclear what "other numbers" actually refers to.
Can you revise the test description to make it more informative?
There was a problem hiding this comment.
There is only one test in this script.
cjyuan
added
Reviewed
2 participants
Learners, PR Template
Self checklist
Changelist
In this PR I implemented the required functions using the TDD approach.
Wrote tests first before implementing the functions
Made the tests pass by writing the correct logic
Refactored the code to improve readability and structure
Ensured all tests pass successfully using Jest
Questions
I would appreciate feedback on:
Whether my TDD workflow is correct
Code structure and readability
Test quality and naming