Skip to content

Birmingham | 26-ITP-May | Ogbemi Mene | sprint 3 | 2-practice TDD coursework exercises#1491

Open
meneogbemi42-bit wants to merge 4 commits into
CodeYourFuture:mainfrom
meneogbemi42-bit:Sprint-3/-Practice-TDD-Coursework-Exercises
Open

Birmingham | 26-ITP-May | Ogbemi Mene | sprint 3 | 2-practice TDD coursework exercises#1491
meneogbemi42-bit wants to merge 4 commits into
CodeYourFuture:mainfrom
meneogbemi42-bit:Sprint-3/-Practice-TDD-Coursework-Exercises

Conversation

@meneogbemi42-bit

Copy link
Copy Markdown

Self checklist

@github-actions

This comment has been minimized.

@meneogbemi42-bit meneogbemi42-bit changed the title Birmingham | 26-ITP-May | Ogbemi Mene sprint 3 | 2-practice tdd coursework exercises Birmingham | 26-ITP-May | Ogbemi Mene | sprint 3 | 2-practice tdd coursework exercises Jul 10, 2026
@meneogbemi42-bit meneogbemi42-bit added Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. 🏕 Priority Mandatory This work is expected labels Jul 10, 2026
@meneogbemi42-bit meneogbemi42-bit changed the title Birmingham | 26-ITP-May | Ogbemi Mene | sprint 3 | 2-practice tdd coursework exercises Birmingham | 26-ITP-May | Ogbemi Mene | sprint 3 | 2-practice TDD coursework exercises Jul 14, 2026

@cjyuan cjyuan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You missed updating two of the six files in the TDD folder.

Also, the PR description is missing a "Changelist" section.

Comment on lines +10 to +13
const pr = new Intl.PluralRules("en-US", { type: "ordinal" });
const suffixes = { one: "st", two: "nd", few: "rd", other: "th" };
const rule = pr.select(n);
return `${n}${suffixes[rule]}`;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting approach. Could you solve it without using any built-in API?

Comment on lines +59 to +65
test("should append 'th' for all other numbers", () => {
expect(getOrdinalNumber(4)).toEqual("4th");
expect(getOrdinalNumber(10)).toEqual("10th");
expect(getOrdinalNumber(20)).toEqual("20th");
expect(getOrdinalNumber(34)).toEqual("34th");
expect(getOrdinalNumber(100)).toEqual("100th");
});

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

To make the test coverage more comprehensive, consider using arrays and loops (or other suitable Jest patterns) to test multiple values, ideally covering numbers that end with every possible digit in this category.

@cjyuan 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 Jul 20, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🏕 Priority Mandatory This work is expected Reviewed Volunteer to add when completing a review with trainee action still to take.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants