London | 25-ITP-Sep | Adnaan Abo | Sprint 3 | coursework/sprint-3-practice-tdd #718

AdnaanA · 2025-09-29T08:55:35Z

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

Changelist

fixed test-case functions and created varies test cases to make sure the testcase function works correctly and passes all test case scenarios.

cjyuan

There are also some exercises in Sprint-3/1-implement-and-rewrite-tests/.

cjyuan · 2025-10-08T16:54:42Z

Sprint-3/2-practice-tdd/get-ordinal-number.js

 function getOrdinalNumber(num) {
-  return "1st";
+
+  if (typeof num !== "number" || !Number.isInteger(num) || num <= 0) {


Can you think of a more concise way to express the condition of this if statement?

Hello,

Thank you for the feedback.

Yes the If statement can be made more concise. Here’s a more compact version:

if (!Number.isInteger(num) || num <= 0) {

cjyuan · 2025-10-08T16:56:32Z

Sprint-3/2-practice-tdd/get-ordinal-number.js

+if (num % 100 >= 11 && num % 100 <= 13) {
+    return num + "th";
+  }


Indentation is a bit off.

Indentation is now fixed

cjyuan · 2025-10-08T16:59:29Z

Sprint-3/2-practice-tdd/get-ordinal-number.test.js

+// Case 7: Identify the ordinal number for 102
+// When the number is 102,
+// Then the function should return "102nd"
+
+test("should return '102nd' for 102", () => {
+  expect(getOrdinalNumber(102)).toEqual("102nd");
+});


To ensure thorough testing, we need broad scenario coverage. Listing individual values, however, can quickly lead to an unmanageable number of test cases.
Instead of writing tests for individual numbers, consider grouping all possible input values into meaningful categories. Then, select representative samples from each category to test. This approach improves coverage and makes our tests easier to maintain.

For example, we can prepare a test for numbers 2, 22, 132, etc. as

test("append 'nd' to numbers ending in 2, except those ending in 12", () => { expect( getOrdinalNumber(2) ).toEqual("2nd"); expect( getOrdinalNumber(22) ).toEqual("22nd"); expect( getOrdinalNumber(102) ).toEqual("102nd"); });

Thank you for the feedback I have grouped all the different teste-cases to categories.

some of the benefits of this approach are:

Scalability — We don't need to test every number, just representatives of each category.

Maintainability — It is easier to read and update tests when logic changes.

Comprehensive — It covers all the functional branches in the logic.

Avoids Redundancy — We won't waste time testing repetitive patterns unnecessarily.

cjyuan · 2025-10-08T17:02:11Z

Sprint-3/2-practice-tdd/repeat.js

 function repeat() {
-  return "hellohellohello";
+  if (arguments.length !== 2) {
+    throw new Error("Function requires exactly two arguments: str and count.");
+  }
+
+  const [str, count] = arguments;
+


Why not just declare the parameters as

function repeat(str, count) { ...

?

Yes we can declare the parameters in the function call and we will not need to declare const [str, count] = arguments;.

cjyuan · 2025-10-08T17:04:16Z

Sprint-3/2-practice-tdd/repeat.js

+    throw new Error("First argument must be a string.");
+  }
+
+  if (typeof count !== "number" || !Number.isInteger(count) || count < 0) {


Similar to the case in get-ordinal-number.js,
can you think of a more concise way to express the condition of this if statement?

There are two other ways that this condition can be expressed concisely.

concise (but potentially less readable):
if (!(Number.isInteger(count) && count >= 0))

and the other concise but more readable option:
if (!Number.isSafeInteger(count) || count < 0)

cjyuan · 2025-10-08T17:08:42Z

In your PR description, the "Changelist" section is missing a header.

cjyuan

Changes look good. I just have a few more suggestions.

Also, in the PR description, can you make the "Changelist" header to look like this?

Changelist

cjyuan · 2025-10-09T10:59:18Z

Sprint-3/2-practice-tdd/get-ordinal-number.test.js


-test("should return '4th' for 4", () => {
+// category 4: all other numbers -> "th"
+test("append 'th' to all other numbers", () => {


When this test fails, the description "all other numbers" is not quite informative; one would need to read the other tests to figure out what numbers are considered as "other numbers".

fixed to more detailed description, "append 'th' to numbers ending in 0, 4–9, or 11–13"

cjyuan · 2025-10-09T11:02:07Z

Sprint-3/2-practice-tdd/get-ordinal-number.test.js

+  expect(getOrdinalNumber(12)).toEqual("12th");
+  expect(getOrdinalNumber(13)).toEqual("13th");
+  expect(getOrdinalNumber(14)).toEqual("14th");
+  expect(getOrdinalNumber(111)).toEqual("111th");
+  expect(getOrdinalNumber(112)).toEqual("112th");
+  expect(getOrdinalNumber(113)).toEqual("113th");


Why test the same numbers again in a separate test?

You are right all the test under category 5: special case for 11, 12, 13 -> "th" can be removed since they are tested in category 4: all other numbers -> "th"

cjyuan · 2025-10-09T14:03:27Z

Changes to the code look good.

I wil mark this PR as complete first.

Can you also address this comment?

AdnaanA · 2025-10-09T14:44:29Z

Changes to the code look good.

I wil mark this PR as complete first.

Can you also address this comment?

✔️ Done

AdnaanA added 3 commits September 27, 2025 10:22

created and tested jest-testcases

889f34e

getordinalnumber test cases have been created and tested to pass all

c40ecef

created test-case for repeat and tested the cases with a pass

f097ae0

AdnaanA added Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. 📅 Sprint 3 Assigned during Sprint 3 of this module labels Sep 29, 2025

cjyuan reviewed Oct 8, 2025

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 8, 2025

AdnaanA added 5 commits October 9, 2025 10:05

made if statement more concise

09a628b

indentation fixed

a87356b

categorised the test-cases

8b8e22f

fixed parameter declaration

b31f116

fixed if statement to be more concise

daf6efa

AdnaanA added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Oct 9, 2025

cjyuan reviewed Oct 9, 2025

View reviewed changes

cjyuan removed the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Oct 9, 2025

AdnaanA added 3 commits October 9, 2025 14:21

removed cat:5 test cases

c7bc598

fixed description for category 4

4183f3b

modified: Sprint-3/2-practice-tdd/get-ordinal-number.test.js

c3eae64

AdnaanA added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Oct 9, 2025

Uh oh!

London | 25-ITP-Sep | Adnaan Abo | Sprint 3 | coursework/sprint-3-practice-tdd #718

Are you sure you want to change the base?

London | 25-ITP-Sep | Adnaan Abo | Sprint 3 | coursework/sprint-3-practice-tdd #718

Uh oh!

Conversation

AdnaanA commented Sep 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changelist

Uh oh!

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

cjyuan commented Oct 8, 2025

Uh oh!

cjyuan left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Changelist

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!

cjyuan commented Oct 9, 2025

Uh oh!

AdnaanA commented Oct 9, 2025

Uh oh!

Uh oh!

AdnaanA commented Sep 29, 2025 •

edited

Loading

cjyuan left a comment •

edited

Loading