Skip to content

London | 26-ITP-Sept | Fung Nin Lee | Sprint 2 | practice-tdd - #1708

Open
leerogerfn wants to merge 17 commits into
CodeYourFuture:mainfrom
leerogerfn:coursework/sprint-3-practice-tdd
Open

leerogerfn wants to merge 17 commits into
CodeYourFuture:mainfrom
leerogerfn:coursework/sprint-3-practice-tdd

Conversation

@leerogerfn

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

written function and tests.

Questions

:)

@leerogerfn leerogerfn added 📅 Sprint 2 Assigned during Sprint 2 of this module Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. labels Oct 14, 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 18, 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, and it would be nice if the following can be improved in future submissions as well:

  • Try not to commit package.json and package-lock.json into the repository. If you don't know how to choose certain files to stage before commit, feel free to ask a volunteer in Saturday class.
  • The indentation of get-ordinal-number.js is a bit hard to read. Please format the code properly in the future. Thank you.

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 of str where it is all the char to count. Would it still count correctly if the char just exists in some part of the str?

Comment thread practice-tdd/count.test.js Outdated
Comment on lines +27 to +32
test("should count multiple occurrences of a character", () => {
const str = "bbbbb";
const char = "a";
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.

There is a special str that would give 0 anyway. Please add that test case as well. Thank you.

@hackertainment hackertainment Oct 20, 2026 •

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I see you added the case of counting "q" in str "codeyourfuture". This is just similar to finding "b" in str"aaaaa". I meant anystr` would always give 0 no matter which character is counted? Please let me know here in the reply if you can't think of it. 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.

could you advise the example? Thank you.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

When str is empty (i.e. str = ""), no matter which character to count, it should return 0. This is a valid test case (but some programs might return error which should be fixed). Your program is correct and just add this particular test case. Thank you.

Comment on lines +34 to +38
test("should append 'th' for numbers 11/12/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.

how about a larger number like 111?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

111 should be 111th but not 111st. Similar to 112th and 113th. Please fix them, thank you.

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?

@hackertainment hackertainment Oct 20, 2026 •

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

"goodbye" is not the special string. Please let me know here in the reply if you can't think of it. 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.

could you advise the special string? Thank you.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The special string is empty string "". When count is greater than or equal to zero, it should return "". When count is a negative number, it should still throw error. Your program is correct, and just add empty string in each of your test suites. Thank you.

@hackertainment hackertainment 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 19, 2026
@leerogerfn leerogerfn 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 20, 2026
@hackertainment

Copy link
Copy Markdown

@hackertainment hackertainment 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 20, 2026
@leerogerfn leerogerfn 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 21, 2026
@hackertainment

Copy link
Copy Markdown

Empty string is considered as boundary case and should normally be included when designing tests.

Also, please fix the get-ordinal-number.js as shown below:

111 should be 111th but not 111st. Similar to 112th and 113th. Please fix them, thank you.

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

Labels

Reviewed Volunteer to add when completing a review with trainee action still to take. 📅 Sprint 2 Assigned during Sprint 2 of this module

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants