London | 26-ITP-January | Khaliun Baatarkhuu | Sprint 3 | Practice tdd. #1182
London | 26-ITP-January | Khaliun Baatarkhuu | Sprint 3 | Practice tdd. #1182khaliun-dev wants to merge 6 commits intoCodeYourFuture:mainfrom
Conversation
Fix countChar function to correctly count occurrences of a character in a string.
Refactor repeatStr function to accept parameters for string and count, and add error handling for invalid count.
khaliun-dev
added
the
Needs Review
favourO
left a comment
favourO
left a comment
There was a problem hiding this comment.
Nice work overall, count and getOrdinalNumber are clear and the tests cover the main cases well. I’ve left 2 comments on repeatStr and it's test to help clean up the final implementation before approval.
| //function repeatStr() { | ||
| // return "hellohellohello"; | ||
| function repeatStr(str, count) { | ||
| if (count >= 0) return str.repeat(count); |
There was a problem hiding this comment.
The main logic is working here. Could you make this a little easier to read by expanding the if / else block, and have another look at the error message? At the moment it feels a bit unfinished, so a clearer message would help someone understand what went wrong.
There was a problem hiding this comment.
function repeatStr(str, count) {
if (count >= 0) {
return str.repeat(count);
} else {
throw new Error("Invalid argument: 'count' must be a non-negative number.");
}
}
module.exports = repeatStr;
|
|
||
| test("should throw an error when count is negative", () => { | ||
| expect(() => repeatStr("hello", -1)).toThrow(); | ||
| }); |
There was a problem hiding this comment.
Nice to see this edge case covered. At the moment the test only checks that an error is thrown. Do you think it would make the test more helpful if it also checked that the error message explains what went wrong?
There was a problem hiding this comment.
test("should throw a helpful error when count is negative", () => {
expect(() => repeatStr("hello", -1))
.toThrow("Count cannot be negative");
});
favourO
added
Reviewed
khaliun-dev
added
the
Needs Review
|
@favourO Thank you for reviewing my PR. Could you, please, have another look? I think i have addressed both issues you have raised, properly. Cheers, Khaliun |
favourO
left a comment
favourO
left a comment
There was a problem hiding this comment.
Nice job,
We are ready to close now.
favourO
added
Complete
2 participants
Self checklist
Changelist
Completed all the exercises from Practice TDD.
Questions
None at the moment.