London | 25-ITP-September | Ammad Ur Rehman | Sprint 3 | 03 Complete Sprint 3 practice TDD coursework #815
Conversation
anosidium
added
the
Needs Review
|
Your PR description contained template fields which weren't filled in. Check you've ticked everything in the self checklist, and that any sections which prompt you to fill in an answer are either filled in or removed. If this PR is not coursework, please add the NotCoursework label (and message on Slack in #cyf-curriculum or it will probably not be noticed). |
2 similar comments
|
Your PR description contained template fields which weren't filled in. Check you've ticked everything in the self checklist, and that any sections which prompt you to fill in an answer are either filled in or removed. If this PR is not coursework, please add the NotCoursework label (and message on Slack in #cyf-curriculum or it will probably not be noticed). |
|
Your PR description contained template fields which weren't filled in. Check you've ticked everything in the self checklist, and that any sections which prompt you to fill in an answer are either filled in or removed. If this PR is not coursework, please add the NotCoursework label (and message on Slack in #cyf-curriculum or it will probably not be noticed). |
anosidium
force-pushed
the
coursework/sprint-3-practice-tdd
branch
from
4574d83 to
4002e46
Compare
jaymes15
added
Review in progress
| @@ -1,5 +1,7 @@ | |||
| function countChar(stringOfCharacters, findCharacter) { | |||
There was a problem hiding this comment.
@anosidium ✅ Great job! Your countChar function correctly counts occurrences and passes all test scenarios.
Using reduce here is neat and expressive. For clarity or performance, you could also consider a for...of loop, but your current solution is clean and idiomatic.
There was a problem hiding this comment.
@anosidium How should we handle cases where the first argument (stringOfCharacters) is an integer and the second (findCharacter) is a string — essentially when the parameters are of the wrong data types?
I believe adding a check for this would make the function more robust. What do you think?
There was a problem hiding this comment.
I agree and updated the countChar() function.
| const char = "z"; | ||
| const count = countChar(str, char); | ||
| expect(count).toEqual(0); | ||
| }); |
There was a problem hiding this comment.
@anosidium I think we can add more test cases to cover additional scenarios — such as how the function behaves with an empty string, a single occurrence, or when the character case differs (e.g., 'A' vs 'a').
It’s good practice to include these kinds of edge cases when writing tests to ensure the function handles all possible inputs robustly.
There was a problem hiding this comment.
Agreed, I've added more test cases.
| @@ -1,5 +1,15 @@ | |||
| function repeat() { | |||
| return "hellohellohello"; | |||
| function repeat(string, count) { | |||
There was a problem hiding this comment.
It could also help to add a short comment above the function describing what it does and the expected parameter types — this improves readability for anyone new to the code.
jaymes15
added
Reviewed
anosidium
added
Needs Review
|
good job getting those suggestions done! |
mjpeet
added
Complete
3 participants
Learners, PR Template
Self checklist
Changelist
I completed all the exercises
Questions