London | 26-ITP-May | Ebrahim Moqbel | sprint 3 | practice tdd - #1595
Open
Ebrahim-Moqbel wants to merge 3 commits into
Open
London | 26-ITP-May | Ebrahim Moqbel | sprint 3 | practice tdd#1595Ebrahim-Moqbel wants to merge 3 commits into
Ebrahim-Moqbel wants to merge 3 commits into
Conversation
This comment has been minimized.
This comment has been minimized.
Ebrahim-Moqbel
added
📅 Sprint 3
This comment has been minimized.
This comment has been minimized.
github-actions
Bot
removed
the
Needs Review
This comment has been minimized.
This comment has been minimized.
1 similar comment
This comment has been minimized.
This comment has been minimized.
Ebrahim-Moqbel
changed the title
Ebrahim-Moqbel
changed the title
Ebrahim-Moqbel
added
the
Needs Review
cjyuan
added
Review in progress
cjyuan
reviewed
Aug 5, 2026
Comment on lines
+1
to
+12
| //counts the number of times a character occurs in a string | ||
| function countChar(stringOfCharacters, findCharacter) { | ||
| return 5 | ||
| let count =0; | ||
| for (let char of stringOfCharacters) { | ||
| if (char === findCharacter) { | ||
| count+=1; | ||
| } | ||
| } | ||
|
|
||
|
|
||
| return count | ||
|
|
Contributor
There was a problem hiding this comment.
-
Code is correct.
-
The spacing around the operators is not quite consistent. Why not use a formatter to keep the code consistently formatted?
-
In a for-of or for-in loop, normally we don't need to reassign the loop variable a different value in the loop, so common practice is to declare the loop variable using
const.
Comment on lines
+6
to
+9
| test("should count the number of times a character occurs in a string", () => { | ||
| expect(countChar('hello world', "o")).toEqual(2); | ||
| expect(countChar('hello world', "l")).toEqual(3); | ||
| expect(countChar('hello world', "z")).toEqual(0); |
Contributor
There was a problem hiding this comment.
Good to test multiple samples.
Comment on lines
+7
to
+19
| // umbers ending with 1 | ||
| if (lastDigit === 1) { | ||
| return num + "st"; | ||
| } | ||
| // numbers ending with 2 | ||
| if (lastDigit === 2) { | ||
| return num + 'nd'; | ||
| } | ||
| // numbers ending with 3 | ||
| if (lastDigit === 3) { | ||
| return num + 'rd'; | ||
| } | ||
| //any other number |
Contributor
There was a problem hiding this comment.
I think the code is clear and readable already without the comments. You could consider removing them.
cjyuan
added
Reviewed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Learners, PR Template
Self checklist
Changelist
completed the TDD files and added some test cases for revision purposes
Questions
N/A