Glasgow | 25-ITP-Sep | Fares Bakhet | Sprint 3 | coursework/sprint-3-practice-tdd#757
Glasgow | 25-ITP-Sep | Fares Bakhet | Sprint 3 | coursework/sprint-3-practice-tdd#757Fares-Bakhet wants to merge 12 commits intoCodeYourFuture:mainfrom
Conversation
| num = num.toString(); | ||
| if ( | ||
| num.slice(-2) === "11" || | ||
| num.slice(-2) === "12" || | ||
| num.slice(-2) === "13" | ||
| ) { | ||
| return num + "th"; | ||
| } else if (num.slice(-1) === "1") { | ||
| return num + "st"; | ||
| return num + "st"; | ||
| } else if (num.slice(-1) === "2") { | ||
| return num + "nd"; | ||
| } else if (num.slice(-1) === "3") { | ||
| return num + "rd"; | ||
| } else { | ||
| return num + "th"; | ||
| } | ||
| } |
There was a problem hiding this comment.
-
There is an unreachable statement (dead code).
-
Note: because of the
returnstatements, usingelseis optional.
There was a problem hiding this comment.
To ensure thorough testing, we need broad scenario coverage. Listing individual values, however, can quickly lead to an unmanageable number of test cases.
Instead of writing tests for individual numbers, consider grouping all possible input values into meaningful categories. Then, select representative samples from each category to test. This approach improves coverage and makes our tests easier to maintain.
For example, we can prepare a test for numbers 2, 22, 132, etc. as
test("append 'nd' to numbers ending in 2, except those ending in 12", () => {
expect( getOrdinalNumber(2) ).toEqual("2nd");
expect( getOrdinalNumber(22) ).toEqual("22nd");
expect( getOrdinalNumber(132) ).toEqual("132nd");
});
Sprint-3/2-practice-tdd/repeat.js
Outdated
| const str = arguments[0]; | ||
| const count = arguments[1]; |
There was a problem hiding this comment.
Why not just declare them as parameters?
|
|
||
| test("should return '101st' for 101", () => { | ||
| expect(getOrdinalNumber(101)).toEqual("101st"); | ||
| test("should return 'th' for the rest numbers", () => { |
There was a problem hiding this comment.
"the rest [of the] numbers" is not very informative by itself; the developer would need to read all the test descriptions to find out what "the rest ..." mean. Can you make this test description more informative?
Sprint-3/2-practice-tdd/repeat.js
Outdated
| //const str = arguments[0]; | ||
| //const count = arguments[1]; |
| } | ||
| return num + "th"; |
There was a problem hiding this comment.
Since you have decided to remove the optional else, why only remove the last else?
cjyuan
left a comment
There was a problem hiding this comment.
Changes look good! Well done.
Learners, PR Template
Self checklist
Changelist
Questions
No questions.