Skip to content

London | 26-ITP-SEP | Russom Gebremeskel | Sprint 2 | Practice TDD - #1723

Open
russom-g wants to merge 67 commits into
CodeYourFuture:mainfrom
russom-g:coursework/sprint-2-practice-tdd
Open

russom-g wants to merge 67 commits into
CodeYourFuture:mainfrom
russom-g:coursework/sprint-2-practice-tdd

Conversation

@russom-g

Copy link
Copy Markdown

Learners, PR Template

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

Changelist

Wrote tests with Jest to me the criteria first. Then implemented the function. Finally tests run to check functions meet the requirements.

@github-actions

This comment has been minimized.

@russom-g russom-g added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Oct 16, 2026
@russom-g russom-g changed the title London | 26 ITP SEP | Russom Gebremeskel | Sprint 2 | Practice TDD London | 26-ITP-SEP | Russom Gebremeskel | Sprint 2 | Practice TDD Oct 22, 2026
@hackertainment hackertainment 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 22, 2026

@hackertainment hackertainment left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Thank you for your coursework. Keep it up.

Comment on lines 13 to 18
test("should count multiple occurrences of a character", () => {
const str = "aaaaa";
const char = "a";
const count = countChar(str, char);
expect(count).toEqual(5);
});

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This is a special case where char appears in the whole string str. Please add a general test case. Thank you.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I have added another test case.

Comment on lines +26 to +31
test("should return 0 for a character with no occurrences", () => {
const str = "aaaaa";
const char = "b";
const count = countChar(str, char);
expect(count).toEqual(0);
});

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This test case is correct. However, there should be a special str which gives 0 anyway. Can you think of it? If so, please add it as well. Thank you.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Would that be an empty string("") ?

Comment on lines +43 to +47
test("should append 'th' for numbers ending with 11, 12 and 13", () => {
expect(getOrdinalNumber(11)).toEqual("11th");
expect(getOrdinalNumber(12)).toEqual("12th");
expect(getOrdinalNumber(13)).toEqual("13th");
});

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

These are numbers exactly equal to 11/12/13, how about larger numbers ended with 11/12/13 ? Please add those tests as well. Thank you.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Done.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Your test cases are correct. Just for you to think a little bit more, besides const str = "hello", what special string should also be added to each of the test cases?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

An empty string? Additional test with empty string input is added.

Thanks for your support with the review and feedback.

@russom-g russom-g added Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. and removed Review in progress This review is currently being reviewed. This label will be replaced by "Reviewed" soon. labels Oct 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants