London | 26-ITP-Jan | Zadri Abdule | Sprint 3 | coursework practice tdd#1271
London | 26-ITP-Jan | Zadri Abdule | Sprint 3 | coursework practice tdd#1271Zadri415 wants to merge 5 commits intoCodeYourFuture:mainfrom
Conversation
This comment has been minimized.
This comment has been minimized.
Zadri415
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.
2 similar comments
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Zadri415
changed the title
Zadri415
added
the
Needs Review
Zadri415
changed the title
ykamal
added
Review in progress
| // implement a function countChar that counts the number of times a character occurs in a string | ||
| const countChar = require("./count"); | ||
| const countChar = require("./count");{ | ||
| } |
There was a problem hiding this comment.
Do you think there is a syntax issue here? How would you ensure your IDE can catch such issues?
Worth checking: file formatting with Priettier
There was a problem hiding this comment.
I think overall this is good. Good job so far 👍
There are a few things you should look into:
- For counting the occurrences of a character in a string, you could use something like
.split()and do a count on the returning array. The benefit is that it's cleaner and probably more performant. - This function could also fail if the string provided doesn't match certain criteria. As a developer, you should account for those. Details in the comment associated.
- There is a syntax issue in the
count.test.jsfile. Details in the comment associated. getOrdinalNumbercould fail if the number is not as expected. Details in the comment associated.
| @@ -1,5 +1,13 @@ | |||
| function countChar(stringOfCharacters, findCharacter) { | |||
There was a problem hiding this comment.
What if the string stringOfCharacters was not provided at all? What if it's undefined, or null?
You should take into account where your code could produce an error and account for that.
How do you think you'd account for those scenarios?
| @@ -1,5 +1,18 @@ | |||
| function getOrdinalNumber(num) { | |||
There was a problem hiding this comment.
Since it's a number, it could be negative or a float (decimal).
Does this function assume that the number is always going to be a positive whole number?
How would you account for those scenarios?
ykamal
added
Reviewed
| // Case 5: ALL other numbers | ||
| // When the number does not end with 1, 2, 3, 11, 12, or 13, | ||
| // Then the function should return a string by appending "th" to the number. | ||
| test("should append 'th' for all other numbers", () => { |
There was a problem hiding this comment.
Might be worth adjusting this test to be more specific, as 'all other numbers' failing won't tell us which numbers they are? Other tests look great!
| if (lastDigits >= 11 && lastDigits <= 13) { | ||
| return num + "th"; | ||
| } | ||
| switch (lastDigits % 10) { |
There was a problem hiding this comment.
Nice use of a switch statement here!
3 participants
Learners, PR Template
Self checklist
Completed the Sprint 3 practice TDD tasks