Scotland | ITP JAN-2026 | Tuan Nguyen | Sprint 3 | 2- Practice-tdd#1258
Scotland | ITP JAN-2026 | Tuan Nguyen | Sprint 3 | 2- Practice-tdd#1258Jacknguyen4438 wants to merge 3 commits intoCodeYourFuture:mainfrom
Conversation
Jacknguyen4438
added
📅 Sprint 3
| if (findCharacters.length < 1) { | ||
| throw new Error("findCharacters must be a single character"); | ||
| } |
There was a problem hiding this comment.
Is findCharacters allowed to be more than one characters?
There was a problem hiding this comment.
Could consider testing a few more samples in this script - higher chance to detect bugs in code.
The original specification did not clearly state whether the character match should be case-sensitive.
Most people would probably assume that it is, but to demonstrate our understanding or clarify the assumption we made,
we could add test cases to convey this. For examples,
- A case to show that the match is case sensitive
- A case to show that the function is expected to work also for non-alphabets
| if(num % 100 === 11 ||num % 100 === 12 || num % 100 === 13 ){ | ||
| return `${num}th`; | ||
| } | ||
| switch( true ){ | ||
| case num % 10 === 1 : return `${num}st`; | ||
| case num % 10 === 2 : return `${num}nd`; | ||
| case num % 10 === 3 : return `${num}rd`; | ||
| default : return `${num}th`; | ||
| } |
There was a problem hiding this comment.
Can you look up the pros and cons between these two styles of expressing the code?
if(num % 100 === 11 ||num % 100 === 12 || num % 100 === 13 ){
const lastTwoDigits = num % 100;
if (lastTwoDigits === 11 || lastTwoDigits === 12 || lastTwoDigits === 13 ) {
?
| } | ||
|
|
||
| // This allow float number to be round into it nearest integer. | ||
| num = Math.round(num); |
There was a problem hiding this comment.
Why do you decide to round the numbers with decimal places? Why not reject them?
| // Case 5: All other numbers | ||
| // When the number does not end with 1, 2, or 3, | ||
| // Then the function should append "th". | ||
| 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?
| switch (true){ | ||
| case repeatCount === 0 : return ""; | ||
| default: return fullString.repeat(repeatCount) ; | ||
| }; |
There was a problem hiding this comment.
What do you think about this writing style?
if (repeatCount === 0) return "":
return fullString.repeat(repeatCount);
cjyuan
added
Reviewed
|
@cjyuan Hello thank you for the review, I see there are many part that need to be fix and improve. I will check all 5 part that you would like me to explain and fix each of them. When I finish I will mention you again to ask for a review. |
2 participants
Learners, PR Template
Self checklist
Changelist
I have done practice and fully understand how to write a test for both normal JS and Jest test.
Questions
No Question.