Glasgow | 25-ITP-SEP | Alaa Tagi | Sprint 3 | Coursework/sprint 3 practice tdd #758
Conversation
Alaa-Tagi
added
📅 Sprint 3
jennethydyrova
added
Review in progress
There was a problem hiding this comment.
Hi @Alaa-Tagi! Great job on translating instructions into functions. It was quite easy to read your PR, which is always a good thing for a reviewer!
A couple of things to note:
- You should try to remove all your testing and scratch work when pushing a PR (mostly console logs in your case).
- Test cases need to be more comprehensive. Think of it this way: when you write different test cases for your functions, you help yourself catch bugs early. For example, currently your
getOrdinalNumberfunction will most likely return"trueth"fornum=true, which I don't think is the expected behavior.
Sprint-3/2-practice-tdd/count.js
Outdated
| console.log(countChar('hello', 'l')); | ||
| console.log(countChar('hello', 'z')); | ||
| console.log(countChar('alaaaaa', 'a')); | ||
|
|
There was a problem hiding this comment.
Let's keep the PR clean and focused. Could you please remove all console logs from your PR?
| // When the function is called with these inputs, | ||
| // Then it should return 0, indicating that no occurrences of the char were found in the case-sensitive str. | ||
|
|
||
| test("should return 0 for no occurrences", () => { |
There was a problem hiding this comment.
This test is good but it doesn't test different possible function inputs. What if the inputs' types are not strings, the char string is longer than the str string, or you have an empty string for either of the inputs, etc.? Testing different inputs will show whether your function accounts for different scenarios, which is what makes a function robust.
| function getOrdinalNumber(num) { | ||
| return "1st"; | ||
| num = num.toString(); | ||
| if (num.slice(-2) === '11' || num.slice(-2) === '12' || num.slice(-2) === '13') { |
There was a problem hiding this comment.
Is there way not to repeat num.slice(-1) and num.slice(-2)?
| test("should return '1st' for 1", () => { | ||
| expect(getOrdinalNumber(1)).toEqual("1st"); | ||
| }); | ||
|
|
There was a problem hiding this comment.
I like these test cases but again, what are other expected/unexpected inputs your function can receive and how will it behave? For example, what if num is undefined, a boolean, etc.? That's just two examples of num being not of a type you expect.
Sprint-3/2-practice-tdd/repeat.js
Outdated
| const str = arguments[0]; | ||
| const count = arguments[1]; |
There was a problem hiding this comment.
Are you sure this is the right way of getting values of function arguments?
| // When the repeat function is called with these inputs, | ||
| // Then it should return the original str without repetition, ensuring that a count of 1 results in no repetition. | ||
|
|
||
| test("should return the original string when count is 1", () => { |
There was a problem hiding this comment.
This test suit are more comprehensive but can you think of other test cases?
There was a problem hiding this comment.
Hi @jennethydyrova i have fixed all the errors that you have mentioned to me.
jennethydyrova
added
Reviewed
Alaa-Tagi
added
Needs Review
2 participants
Learners, PR Template
Self checklist
Changelist
I have created a PR with completed tasks as required.
Questions
No questions