Skip to content

London | 26-ITP-Sep | Maharit Belai | Sprint 2| Practice TDD - #1729

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

MaharitB wants to merge 57 commits into
CodeYourFuture:mainfrom
MaharitB:coursework/sprint-3-practice-tdd

Conversation

@MaharitB

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

I practiced writing a code based on Test driven development.

Questions

@mayowaolokun mayowaolokun 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
@MaharitB MaharitB 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
@mayowaolokun mayowaolokun 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 25, 2026

@mayowaolokun mayowaolokun 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 the updates, this is great progress. The main issues from the first review have been addressed: the 111, 112 and 113 cases now work, the zero case in repeatStr returns an empty string, and the test names are much clearer.

I’ve left a few smaller comments.
You’re very close, so keep up the good work!

Comment thread practice-tdd/count.test.js Outdated
// When the function is called with these inputs,
// Then it should return 0, indicating that no occurrences of `char` were found.

test(" Should count 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.

“Should count 0” is clearer than before. It might help to describe the scenario, for example, that the character doesn’t appear in the string. There’s also still a space at the start of the name.

Comment thread practice-tdd/count.test.js Outdated
const count = countChar(str, char);
expect(count).toEqual(1);
});
test("should count multiple occurences of character empty space", () => {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

“Empty space” could be confused with an empty string. Would “space character” describe this test more clearly?

Comment thread practice-tdd/count.test.js Outdated
const count = countChar(str, char);
expect(count).toEqual(2);
});
test("should count multiple occurences of character in an empty string", () => {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Nice addition. Since this test expects 0, it might be worth removing “multiple occurrences” from the name.

Comment thread practice-tdd/count.test.js Outdated
const count = countChar(str, char);
expect(count).toEqual(0);
});
test("should count multiple occurences of character in a sentence", () => {

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 seems to share its name with the one on line 52, so it might be worth giving it a unique name that describes its scenario.

Comment thread practice-tdd/get-ordinal-number.js Outdated
throw new Error("Invalid Input");
}

if (numStr.slice(-1) == "1" && numStr.slice(-2) != 11) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

numStr.slice(-2) gives you a string, but it’s compared with the number 11. This currently works because != converts types before comparing. What would happen if you switched to !==, as suggested earlier? It might be worth comparing like with like so the code doesn’t depend on type conversion. Applicable to lines 11 and 14.

Comment thread practice-tdd/repeat-str.js Outdated
count = Math.floor(count);

if (str === " ") {
throw new Error("String shoild not be empty");

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Small typo: “shoild” should be “should”.


test("should repeat the string count times", () => {
expect(() => repeatStr("hello", -1)).toThrow("Negative counts are not valid");
});

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 adding this test. One thing to check: the input here is a single space " " rather than an empty string "". Please see my comment on line 9 of repeat-str.js, as the two are linked.

Comment thread practice-tdd/repeat-str.test.js Outdated
expect(() => repeatStr("hello", -1)).toThrow("Negative counts are not valid");
});
test(`should throw an error when string is empty`, () => {
expect(() => repeatStr(" ", 1)).toThrow("String shoild not be empty");

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 test name says “empty string”, but the input is a space. Once you’ve decided whether an empty string or a space should be an error, it might be worth updating this test to match. Also, if you fix the “shoild” typo in the error message, remember to update it here too.

Comment on lines -18 to -23

// Case: handle count of 1:
// Given a target string `str` and a `count` equal to 1,
// When the repeatStr function is called with these inputs,
// Then it should return the original `str` without repetition.

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 Given/When/Then comments for the count of 1 and count of 0 cases are still removed. It might be worth restoring them, as you’ve done for the other cases.

Comment thread practice-tdd/repeat-str.test.js Outdated
expect(repeatedStr).toEqual("");
});
//case: Handle decimal count:
test("should repeat the stringthe whole number in the count times", () => {

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’s a missing space in “stringthe”. It might also help to make the name clearer, for example, by saying that a decimal count is rounded down.

@mayowaolokun mayowaolokun 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 25, 2026
@MaharitB MaharitB 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 28, 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