London | 25-ITP-Sep | Payman Issa Baiglu | sprint 3 | 2-practice-tdd #883

PaymanIB · 2025-11-15T15:34:38Z

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

Tests and functions implemented and tested.

…numbers

… add corresponding tests

github-actions · 2025-11-15T15:34:56Z

Your PR's title isn't in the expected format.

Please check the expected title format, and update yours to match.

Reason: Wrong number of parts separated by |s

If this PR is not coursework, please add the NotCoursework label (and message on Slack in #cyf-curriculum or it will probably not be noticed).

github-actions · 2025-11-15T15:35:02Z

Your PR's title isn't in the expected format.

Please check the expected title format, and update yours to match.

Reason: Wrong number of parts separated by |s

If this PR is not coursework, please add the NotCoursework label (and message on Slack in #cyf-curriculum or it will probably not be noticed).

github-actions · 2025-11-15T15:51:56Z

Your PR's title isn't in the expected format.

Please check the expected title format, and update yours to match.

Reason: Sprint part (courseworksprint-3) doesn't match expected format (example: 'Sprint 2', without quotes)

If this PR is not coursework, please add the NotCoursework label (and message on Slack in #cyf-curriculum or it will probably not be noticed).

github-actions · 2025-11-15T15:52:32Z

Your PR's title isn't in the expected format.

Please check the expected title format, and update yours to match.

Reason: Sprint part (coursework-sprint-3) doesn't match expected format (example: 'Sprint 2', without quotes)

If this PR is not coursework, please add the NotCoursework label (and message on Slack in #cyf-curriculum or it will probably not be noticed).

github-actions · 2025-11-15T15:53:44Z

Your PR's title isn't in the expected format.

Please check the expected title format, and update yours to match.

Reason: Sprint part (sprint-3) doesn't match expected format (example: 'Sprint 2', without quotes)

If this PR is not coursework, please add the NotCoursework label (and message on Slack in #cyf-curriculum or it will probably not be noticed).

cjyuan · 2025-11-21T09:34:16Z

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

+  const v = num % 100;
+  console.log(v);


It's best practice to give variables meaningful names. Can you think of a more meaningful name to replace v?

Can you keep the code clean by removing all debugging code?

Thanks @cjyuan. I updated the V variable to date to make it more meaningful. also removed the console.logs to make it more clean.

cjyuan · 2025-11-21T09:43:13Z

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

 test("should return '1st' for 1", () => {
  expect(getOrdinalNumber(1)).toEqual("1st");
 });
+
+test("should return '2nd' for 2", () => {
+  expect(getOrdinalNumber(2)).toEqual("2nd");
+});
+
+test("should return '3rd' for 3", () => {
+  expect(getOrdinalNumber(3)).toEqual("3rd");
+});   
+test("should return '24th' for 24", () => {
+  expect(getOrdinalNumber(24)).toEqual("24th");
+});   
+test("should return '11th' for 11", () => {
+  expect(getOrdinalNumber(11)).toEqual("11th");
+});


To ensure thorough testing, we need broad scenarios that cover all possible cases.
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(132) ).toEqual("132nd"); });

Can you update the tests in this script to make it comprehensive so that the test categories can cover all valid numbers?

cjyuan · 2025-11-21T09:47:28Z

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

+  if (count < 0) {
+    return "Count must be a non-negative integer";
+  }


How would the caller distinguish the result of the following two function calls?

repeat("Count must be a non-negative integer", 1)

repeat("", -1)

Both function calls return the same value.

Thanks, I see your point. I changed it so it throw error.

cjyuan · 2025-11-21T09:48:43Z

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

+  for (let i = 0; i < count; i++) { 
+    return str.repeat(count);
+  } 


This loop will always iterate exactly once. Do you know why?

Good point. That's because return is inside the loop. so I changed it.

cjyuan · 2025-11-21T09:49:15Z

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

+test("should return a message for negative count", () => {
+  const str = "error";
+  const count = -2;
+  const repeatedStr = repeat(str, count);
+  expect(repeatedStr).toEqual("Count must be a non-negative integer");
+});


If you modified repeat() to throw an error when count is negative, and you wanted to test if the function can throw an error as expected, you can use .toThrow(). You can find out more about how to use .toThrow() here: https://jestjs.io/docs/expect#tothrowerror (Note: Pay close attention to the syntax of the example)

Thanks for bring this up. I changed it so it will throw error.

…ove unnecessary console logs

…n and enhance ordinal number tests with clearer descriptions and additional cases

…returning a message

…intain error handling for negative counts

…turning a message

PaymanIB · 2025-11-23T13:44:20Z

Hi @cjyuan , thanks for reviewing my PR. I changed the the code so it covers the points that you mentioned can you please check if they are OK? Thanks and regards.

cjyuan

Can you also revert the changes made to

Sprint-2/1-key-erros/0.js
Sprint-3/1-implement-and-rewrite-tests/implement/3-get-card-value.js

and remove Project-CLI-Treasure-Hunt?

cjyuan · 2025-11-23T14:20:06Z

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

 function getOrdinalNumber(num) {
-  return "1st";
+  const suffixes = ["th", "st", "nd", "rd"];
+  const date = num % 100;


Why named the variable date? The last two digits do not represent a date.

Thanks for pointing that out. yes you are absolutely right! I was in class yesterday and couldn't focus enough. I changed it to a proper Variable name.

cjyuan · 2025-11-23T14:23:06Z

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

+test("append 'th' to numbers ending in 4, except those ending in 14", () => {
+  expect(getOrdinalNumber(4)).toEqual("4th");
+  expect( getOrdinalNumber(24) ).toEqual("24th");
+    expect( getOrdinalNumber(134) ).toEqual("134th");
+});   
+test("should return '11th' for 11", () => {
+  expect(getOrdinalNumber(11)).toEqual("11th");
+});


The tests on lines 11-26 are good.

Can you improve the tests on 27-34?

Why differentiate numbers ending in 4 from numbers that ending in 14?

What about numbers ending in other digits?

Why prepare a test just for 11?

Thanks, I tried to improve the tests on line 27-34. also added a test for other numbers.
11 is exception as it ends with 1 but will get "th", so tested separately.

… for better coverage

PaymanIB · 2025-11-23T15:31:58Z

Hi @cjyuan , Many thanks for reviewing my PR. I tried a lot yesterday with the help of 3 volunteers to remove the extras which I don't know how got there in the first place. the best we could do was to made anew branch, which apparently didn't fix the problem. However, as I see those files are all from sprint-3.
I fixed the points you mentioned. can you please check if I've done correctly. kind regards,

cjyuan · 2025-11-23T15:32:34Z

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

+test("append 'th' to numbers ending in 4", () => {
+  expect(getOrdinalNumber(4)).toEqual("4th");
+  expect( getOrdinalNumber(24) ).toEqual("24th");
+    expect( getOrdinalNumber(134) ).toEqual("134th");
+});   
+test("should return '11th' for 11", () => {
+  expect(getOrdinalNumber(11)).toEqual("11th");
+});
+test("append 'th' to numbers which are not ending in 1, 2, 3", () => {
+  expect(getOrdinalNumber(5)).toEqual("5th");
+  expect( getOrdinalNumber(29) ).toEqual("29th");
+    expect( getOrdinalNumber(138) ).toEqual("138th");
+});   


Isn't the test categories on 35-39 include numbers ending in 4?

If 11 is a special case, then what about 12, 13, 111, 212, etc.?

Now I got it! Thanks for pointing me that way. 11 is already covered and don't need a separate test, as all other numbers do.

…t tests for 4 and 11

cjyuan · 2025-11-23T20:37:48Z

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

 test("append 'th' to numbers which are not ending in 1, 2, 3", () => {
  expect(getOrdinalNumber(5)).toEqual("5th");
-  expect( getOrdinalNumber(29) ).toEqual("29th");
+  expect( getOrdinalNumber(24) ).toEqual("24th");
    expect( getOrdinalNumber(138) ).toEqual("138th");
 });   


All the test categories now cover all numbers, but within this category, the samples being used are not very comprehensive. Can you think of what numbers you could have also tested?

Payman IB added 3 commits November 15, 2025 14:28

Implement countChar function and add test for non-existent character

92afa66

Refactor getOrdinalNumber function and expand test cases for ordinal …

9e894a7

…numbers

Implement repeat function with error handling for negative counts and…

ded405e

… add corresponding tests

PaymanIB added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Nov 15, 2025

cjyuan reviewed Nov 21, 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 Nov 21, 2025

Payman IB added 6 commits November 22, 2025 12:27

Refactor getOrdinalNumber function to improve variable naming and rem…

a124398

…ove unnecessary console logs

Update getCardValue function to check for specific card representatio…

e7d7fff

…n and enhance ordinal number tests with clearer descriptions and additional cases

reverted to original

fbacf4a

Fix repeat function to throw an error for negative counts instead of …

3585be7

…returning a message

Refactor repeat function to correctly handle string repetition and ma…

bbc996c

…intain error handling for negative counts

Update negative count test to ensure it throws an error instead of re…

5ee3f6b

…turning a message

PaymanIB 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 Nov 23, 2025

cjyuan reviewed Nov 23, 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 Nov 23, 2025

Refactor getOrdinalNumber function for clarity and improve test cases…

354b5ac

… for better coverage

PaymanIB 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 Nov 23, 2025

cjyuan reviewed Nov 23, 2025

View reviewed changes

Fix test case description for numbers ending in 1 and remove redundan…

fdc1b1a

…t tests for 4 and 11

cjyuan reviewed Nov 23, 2025

View reviewed changes

cjyuan added Complete Volunteer to add when work is complete and all review comments have been addressed. and removed Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. labels Nov 23, 2025

Uh oh!

London | 25-ITP-Sep | Payman Issa Baiglu | sprint 3 | 2-practice-tdd #883

Are you sure you want to change the base?

London | 25-ITP-Sep | Payman Issa Baiglu | sprint 3 | 2-practice-tdd #883

Uh oh!

Conversation

PaymanIB commented Nov 15, 2025

Uh oh!

github-actions bot commented Nov 15, 2025

Uh oh!

github-actions bot commented Nov 15, 2025

Uh oh!

github-actions bot commented Nov 15, 2025

Uh oh!

github-actions bot commented Nov 15, 2025

Uh oh!

github-actions bot commented Nov 15, 2025

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!

PaymanIB commented Nov 23, 2025

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!

PaymanIB commented Nov 23, 2025

Uh oh!

cjyuan Nov 23, 2025 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

cjyuan Nov 23, 2025 •

edited

Loading