London|26-ITP-Sep|Abdennour Hachemi|Sprint 2 |Practice TDD by AbdennourHachemi · Pull Request #1701 · CodeYourFuture/Module-Structuring-and-Testing-Data

AbdennourHachemi · 2026-10-13T22:29:20Z

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-1060

valentina-starr1 · 2026-10-14T05:40:15Z

Thanks for working on this..The core exercise implementations look solid overall, but i can see one blocking issue regarding version control configuration that needs to be addressed before merging.

.gitignore Fix (Blocking)
practice-tdd/.gitignore: Please remove package.json and package-lock.json from .gitignore. These files must be tracked in the repository to maintain consistent dependencies, scripts, and build environments across machines.
Actionable Improvements & Refactoring
practice-tdd/repeat-str.js: Returning an error string ("Error! negative counts...") on negative inputs differs from idiomatic JS and the exercise intent. Please throw an Error instead (e.g., throw new Error(...)) and update repeat-str.test.js to use expect(() => ...).toThrow().

practice-tdd/get-ordinal-number.js: The regex logic works, but switching to numeric modulo operations (num % 100 and num % 10) would make the branching much clearer and easier to maintain.

practice-tdd/get-ordinal-number.test.js: Consider adding test cases for 3-digit teen exceptions (e.g., 111, 112, 113) to ensure the suffix rules scale properly.

practice-tdd/count.test.js: Minor nit - @jest/globals imports aren't needed since Jest provides them globally, and there's a small typo in numberOfOcurrence (numberOfOccurrence).

Once package.json and package-lock.json are un-ignored and the error-throwing behavior is updated, this should be good to go.

AbdennourHachemi · 2026-10-14T21:59:28Z

Thanks @valentina , i have fixed my code according to your review . please have a look again and see if anything else needs correction.

hey-hammad

Great implementation overall, a couple minor issues needs attention. Thanks

hey-hammad · 2026-10-20T14:16:32Z

@@ -0,0 +1,2 @@
+node_modules


a .gitignore file already exist in the root of the repo which is enough.

hey-hammad · 2026-10-20T14:24:56Z

  expect(getOrdinalNumber(21)).toEqual("21st");
  expect(getOrdinalNumber(131)).toEqual("131st");
 });
+


a test case handling the special cases like 11,12,13,101 & 103 will make this suit more robust.

cjyuan

Implementation is correct.

Some of the tests could be refined.

cjyuan · 2026-10-20T14:22:32Z

Could consider testing multiple samples in each test category - higher chance to detect bugs in code.

Empty string is often a good edge case to test.

When a specification is ambiguous, programmers may need to make reasonable assumptions based on their understanding of the requirements. In this case, the original specification did not explicitly state whether character matching should be case-sensitive. Although most people would likely assume that it is, adding test cases can help document this assumption and make our interpretation of the specification explicit.

Similarly, we should consider adding tests for any other assumptions made during implementation to clarify the intended behavior and ensure these assumptions are consistently understood and validated.

cjyuan · 2026-10-20T14:30:05Z

+// Case 4: Numbers ending with any other number (but not 1,2 or 3)
+// When the number ends with 4,5,6 ,7 ,8 or 9 ,
+// Then the function should return a string by appending "th" to the number.
+test("should append 'th' for numbers ending with 4,5,6 ,7 ,8 or 9 ", () => {


Are all "other digits" covered in this category?

cjyuan · 2026-10-20T14:34:06Z

+// When the number ends with ,(111, 112,113)
+// Then the function should return a string by appending "th" to the number.
+test("should append 'th' for numbers ending with ", () => {


The test description seems incomplete.

When a comment repeats information already conveyed in the test description, consider removing it to avoid redundancy.

cjyuan · 2026-10-28T15:35:15Z

+test("should append 'th' for numbers in 100's ending with 11, 12 , 13", () => {
+  expect(getOrdinalNumber(111)).toEqual("111th");
+  expect(getOrdinalNumber(112)).toEqual("112th");
+  expect(getOrdinalNumber(113)).toEqual("113th");
+});


Why limit the cases to numbers in the 100's?

cjyuan · 2026-10-28T15:36:34Z

+// Case 6:A test case handling the special cases like 11,12,13,101 & 103
+
+test("should append 'th' for numbers for 11,12,13 and 'st' for 101 and 'rd' for 103", () => {
+  expect(getOrdinalNumber(11)).toEqual("11th");
+  expect(getOrdinalNumber(12)).toEqual("12th");
+  expect(getOrdinalNumber(13)).toEqual("13th");
+  expect(getOrdinalNumber(101)).toEqual("101st");
+  expect(getOrdinalNumber(103)).toEqual("103rd");
+});


Why are these five numbers considered special cases? What do you mean by "special"?

cjyuan · 2026-10-28T15:39:25Z

The tests in this script do not yet covered all valid cases.

cjyuan · 2026-10-28T15:43:16Z

+  //handle a non number inputs
+  if (!num) {
+    return "";
+  }


What do you mean by a non-number? !num is not a proper way to check if num is a number or not.

Note: You could assume the parameter is always a number in this exercise.

AbdennourHachemi added 7 commits October 13, 2026 23:11

updating count.js file

038bb6a

solve excersise count.test.js

7f77ed1

solve excersise get-ordinal-number.js

eafa02b

solve excersise get-ordinal-number.test.js

d48367c

solve excersise repeat-str.js

81f14c5

solve excersise repeat-str.test.js

e69b86d

addinkg .gitignore

fa692ca

AbdennourHachemi added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Oct 13, 2026

AbdennourHachemi added 5 commits October 14, 2026 21:31

remove pakage.json and lock files from gitignore

119d10a

adding toThrow method

c6ff944

update get-ordinal exercise

d5f0650

remove unnessarry import from count.test.js

fec0df1

correcting spelling typo

cfd965b

cjyuan 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 20, 2026

hey-hammad suggested changes Oct 20, 2026

View reviewed changes

hey-hammad 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 20, 2026

cjyuan reviewed Oct 20, 2026

View reviewed changes

AbdennourHachemi added 6 commits October 26, 2026 21:26

adding test case handling the special cases like 11,12,13,101 & 103

c5e9432

delete gitignore file

9221e65

adding multiple sample test in each category

37f772e

adding an empty string test case

a450f92

remove repeated comments to avoid redendancy

64feba5

give a better description to test cases

d31066a

AbdennourHachemi added Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. and removed Reviewed Volunteer to add when completing a review with trainee action still to take. labels Oct 26, 2026

running prettier to fromat the code

5081760

cjyuan reviewed Oct 28, 2026

View reviewed changes

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 28, 2026

Conversation

AbdennourHachemi commented Oct 13, 2026

Task code

Uh oh!

valentina-starr1 commented Oct 14, 2026

Uh oh!

AbdennourHachemi commented Oct 14, 2026

Uh oh!

hey-hammad left a comment

Choose a reason for hiding this comment

Uh oh!

hey-hammad Oct 20, 2026

Choose a reason for hiding this comment

Uh oh!

hey-hammad Oct 20, 2026

Choose a reason for hiding this comment

Uh oh!

cjyuan left a comment

Choose a reason for hiding this comment

Uh oh!

cjyuan Oct 20, 2026

Choose a reason for hiding this comment

Uh oh!

cjyuan Oct 20, 2026

Choose a reason for hiding this comment

Uh oh!

cjyuan Oct 20, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cjyuan Oct 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cjyuan Oct 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cjyuan Oct 28, 2026

Choose a reason for hiding this comment

Uh oh!

cjyuan Oct 28, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

cjyuan Oct 28, 2026 •

edited

Loading

cjyuan Oct 28, 2026 •

edited

Loading