London | 26-ITP-Jan | Carlos Abreu | Sprint 3 | Practice TDD Coursework

[carlosyabreu]

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

Changelist

PR for Sprint-3 of Practice TDD Coursework

[cjyuan]

Function implementation is solid, especially on validating the parameters.
Test cases are comprehensive! Well done!

I think some of the test descriptions could be made more informative.

[cjyuan]

The person implementing the function may not immediately understand what "handle" means. It would be clearer to describe the expected behavior directly in the test description, such as specifying what the function should return.

Can you check which test descriptions (in all three test.js files) can be made more informative?

[carlosyabreu]

Evening @cjyuan
Thanks for the feedback.
I'm not sure if you want me to implement all the code part or just to highlight on the test descriptions how to make it more informative.
Based on my understanding I think that's the second option to make the code more informative so I'll not write all the code implementation here but just the portion of the code that show case it differently using the description below:

1.) countChar.test.js

// Instead of: "should handle empty string"
test("should return 0 when string is empty", () => {
// ...
});

// Instead of: "should handle single occurrence"
test("should return 1 when character appears once in the string", () => {
// ...
});

// Instead of: "should handle case sensitivity"
test("should treat lowercase and uppercase characters as different", () => {
// ...
});

// Instead of: "should handle invalid inputs gracefully"
test("should return 0 when first argument is not a string", () => {
// ...
});

test("should return 0 when second argument is not a string", () => {
// ...
});

test("should return 0 when second argument contains multiple characters", () => {
// ...
});

2.) get-ordinal-number.test.js

This test file I think already has good descriptive names.
Definitely, the descriptions clearly state what behavior is expected.
I don't think any changes needed here.

3.) repeat-str.test.js

/ Instead of: "should handle empty string"
test("should return empty string when input string is empty", () => {
// ...
});

// Instead of: "should handle string with spaces"
test("should preserve spaces when repeating the string", () => {
// ...
});

// Instead of: "should throw error when first argument is not a string"
test("should throw error if first argument is not a string", () => {
// ...
});

[cjyuan]

In TDD, the test description (or test name) should clearly explain what behavior the function should exhibit.
The goal is that someone reading the test can understand the requirement without reading another spec.

A common format is: should <expected_behavior> when <condition>.

You proposed descriptions are much clearer.

No changes required as you have demonstrated you understand how to make the test description informative.

[cjyuan]

One of the check here is redundant.

[carlosyabreu]

Looking at this code again, the redundant check is the !Number.isInteger(number) condition in the 14th line.

The integer check is necessary for the logic to work correctly (since ordinal numbers only make sense for integers).

[cjyuan]

!Number.isInteger(number) is not the redundant check here.

[cjyuan]

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?

[carlosyabreu]

Sure.

// Case 5: All other numbers (should get "th")
describe("should append 'th' for numbers that don't fit special cases", () => {
test("numbers ending with 0", () => {
expect(getOrdinalNumber(0)).toEqual("0th");
expect(getOrdinalNumber(10)).toEqual("10th");
expect(getOrdinalNumber(20)).toEqual("20th");
expect(getOrdinalNumber(100)).toEqual("100th");
expect(getOrdinalNumber(1000)).toEqual("1000th");
});

[cjyuan]

This work, and it would create a very comprehensive test. The downside is, you will need a test for each of the digits 0, 4, ..., 9.

I was thinking in the direction of replacing "all other numbers" by a detailed description of the numbers, like "numbers ending with 0, 4, 5, 6, 7, 8, or 9".