London | 26-ITP-May | Dagim Daniel | Sprint 3 | Practice-tdd#1480
London | 26-ITP-May | Dagim Daniel | Sprint 3 | Practice-tdd#1480Dagim-Daniel wants to merge 1 commit into
Conversation
Dagim-Daniel
added
Needs Review
tiakavousi
added
Review in progress
| return 5 | ||
| let count = 0; | ||
| str = stringOfCharacters | ||
| char = findCharacter |
There was a problem hiding this comment.
Great function, this works perfectly!
Quick question on lines 3 and 4:
str and char are declared without any keywords. What happens to variables in JavaScript when we don't use let or const?
also since we already have stringOfCharacters and findCharacter passed into the function as arguments, do we need to assign them to new variables?
What would happen if you used the arguments directly in your loop?
There was a problem hiding this comment.
we don't need to assign them to new variables, i just used it to help me with the test cases. -
| const count = countChar(str, char); | ||
| expect(count).toEqual(2); | ||
| }); | ||
| // Scenario: No Occurrences |
There was a problem hiding this comment.
You have the setup comments for a 'No Occurrences' scenario, but the test block is missing. maybe you've forgotten to write this one?
There was a problem hiding this comment.
Nice job on these tests!
Since you are already thinking about edge cases like that, what are some other tricky inputs we might want to test for? (For example: what happens if someone passes in an empty string, or wants to count empty spaces?
| function getOrdinalNumber(num) { | ||
| return "1st"; | ||
| const s = ["th", "st", "nd", "rd"]; | ||
| const lastDigits = num % 100; |
There was a problem hiding this comment.
Nice job on the logic
Just a quick formatting note: the indentations here in this file and the others can be improved.
Have you considered setting up a formatter like Prettier in your IDE to handle these automatically?
There was a problem hiding this comment.
yeah i do install the prettier extension, just failed to make it auto-format on save. thanks i will fix it.
| expect(repeatedStr).toEqual("ITPITPITPITPITP"); | ||
| }); | ||
|
|
||
| // Case: handle count of 1: |
There was a problem hiding this comment.
I'm assuming these are some leftover comments that you can safely delete since you already wrote an awesome test for count = 1 up above!
There was a problem hiding this comment.
Imagine one of these tests fails and you're reading that red output in the console. Since all the descriptions are identical right now (both here and in count.test.js), how could we update the test names to be more self-explanatory so you immediately know exactly what broke?
There was a problem hiding this comment.
you are right this could be fix, maybe by making clear test description.
2 participants
Learners, PR Template
Self checklist
Changelist
Sprint 3 all tasks under practice-tdd has been done.