-
-
Notifications
You must be signed in to change notification settings - Fork 241
West Midlands | 25 Sep ITP | Iswat Bello | Sprint 3 | 2 Practice TDD #798
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
West Midlands | 25 Sep ITP | Iswat Bello | Sprint 3 | 2 Practice TDD #798
Conversation
|
Your PR's title isn't in the expected format. Please check the expected title format, and update yours to match. Reason: Wrong number of parts separated by |s 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's title isn't in the expected format. Please check the expected title format, and update yours to match. Reason: Wrong number of parts separated by |s 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's title isn't in the expected format. Please check the expected title format, and update yours to match. Reason: Wrong number of parts separated by |s 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's title isn't in the expected format. Please check the expected title format, and update yours to match. Reason: Wrong number of parts separated by |s 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). |
jennethydyrova
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hi @Iswanna! Good start on the code in this PR! There are a few things to consider while writing unit tests:
- You should try to cover more test cases to see how your function behaves in different scenarios.
- You should test edge cases like empty strings, undefined inputs, different data types, etc. This helps catch bugs early and makes your function more robust.
| // And a character char that does not exist within the case-sensitive str, | ||
| // 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 occurence of a character", () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good start but this is would not be considered as a good test suite as it covers just one behavior. Good test suite should cover different scenarios based on what type of arguments you pass to the function.
| @@ -1,5 +1,12 @@ | |||
| function getOrdinalNumber(num) { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, this function is not passing the acceptance criteria. Please try to pass different values to the function and see if it returns what you expect.
| expect(getOrdinalNumber(1)).toEqual("1st"); | ||
| }); | ||
|
|
||
| test("should return '11th' for 11", () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as a previous comment regarding very slim test suite. Try to think of more test cases.
| // Given a target string str and a count equal to 1, | ||
| // 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 input with no repetition", () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These unit tests are good, great job on writing them! Now I would like you to think of more test cases, where the values passed to the function are not what you expected. What if you pass integer 3 as valueToRepeat or undefined as numOfTimes, etc.?
|
Hi @jennethydyrova. Thank you so much for all the feedback. I will work on all of them and update all the necessary files. |
Add a test to ensure countChar throws an error when more than one character is passed as the 'char' argument, returning a descriptive invalid input message.
…r counts and repeat strings, numbers, booleans, null, undefined, arrays, and objects
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @Iswanna! Really great job on writing unit tests and improving your functions. There are couple of common issues in the functions you wrote. I left comments on the countChar function but those comments are applicable to other functions as well.
- You should try to return early from the function if the arguments are invalid. This makes your function much cleaner and there is really no need to execute the rest of the code if function input is invalid.
- You should be throwing error or at least returning error message without assigning it to a variable that was created to hold something else. This is semantically incorrect, please try to read the function out loud and think if assigning error message to a variable that should hold something else make your function easier or harder to read.
- When you find yourself repeating same code over and over again, try to think how it can be combined into more general structures or variables. Having similar if conditions can be combined into one or having multiple calculations of the same number can be saved in a variable or moved to function.
Something I want to highlight here is how you write commit messages. They are extremely clean and nicely written, really great job on it! If your were working in a team, your teammates would appreciate it a lot as it's very easy to track your changes.
| function countChar(stringOfCharacters, findCharacter) { | ||
| return 5 | ||
| let characterOccurrence = ""; | ||
| if ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's usually better to return early if inputs are invalid, it's called early return. Can you think of a way to implement this in your function?
Sprint-3/2-practice-tdd/count.js
Outdated
| if (findCharacter.length === 1) { | ||
| characterOccurrence = stringOfCharacters.split(findCharacter).length - 1; | ||
| } else { | ||
| characterOccurrence = "invalid input: Input just one character"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should be throwing error or returning error message in the cases like this. Semantically, characterOccurrence = "invalid input: Input just one character"; is incorrect because characterOccurrence is not an error, it's a variable that should hold occurrence of a certain character.
| function getOrdinalNumber(num) { | ||
| return "1st"; | ||
| let ordinalNum; | ||
| if (Number.isInteger(num) === true) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, can you think of ways to return early if function argument is invalid?
| return "1st"; | ||
| let ordinalNum; | ||
| if (Number.isInteger(num) === true) { | ||
| if ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, this is a good improvement on the previous version but it could be simplified a lot.
- Do you need to explicitly check every single number with
num % 10 === x? You are right to have anum % 100check because it handles the 11–13 exceptions correctly, but the rest doesn't look necessary. - You could reduce repetition of the code here. Can you think of the way to not recalculate the remained for the same number over and over again?
Sprint-3/2-practice-tdd/repeat.js
Outdated
| repeatedValue = "Negative number invalid"; | ||
| } else if(!Number.isInteger(numOfTimes)) { | ||
| repeatedValue = "Invalid count: count should be an integer" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is good but you could perhaps simplify it by combining these conditions, because you simply need a positive integer as count number.
Hi @jennethydyrova. Thank you so much for the detailed feedback and for taking the time to review my code! I really appreciate your comments. Thanks again for the thoughtful review and guidance. |
…move redundant code
…and remove redundant code
…ould be an integer' in Jest test case
|
Hi @jennethydyrova. I have made corrections to all the necessary files as you suggested. Thank you so much for the detailed feedback. |
|
Your PR couldn't be matched to an assignment in this module. Please check its title is in the correct format, and that you only have one PR per assignment. 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). |
jennethydyrova
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @Iswanna! Very good iteration on the last version! I left couple of comments, some of them are optional and you don't need to change anything to get your PR accepted but I think it would be useful for you to try to think of better ways of writing some parts.
| const char = findCharacter.toLowerCase(); | ||
|
|
||
| // Check that only one character is passed | ||
| if (char.length !== 1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is optional but you can do this check before L10
| const lastDigit = absNum % 10; | ||
|
|
||
| // Handle special cases: 11th, 12th, 13th | ||
| if (lastTwo === 11 || lastTwo === 12 || lastTwo === 13) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Optional again, can you think of ways to make this if condition simpler?
| if (!Number.isInteger(numOfTimes)) { | ||
| return "Invalid count: count should be an integer"; | ||
| } | ||
| if (numOfTimes < 0) { | ||
| return "Negative number invalid"; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One more optional comment, you should try to keep your error messages consistent in style. If you say Invalid x: x must be an integer, then you should have Invalid x: x must be a positive integer. Here you might see that perhaps you can combine this if condition into something that would return Invalid x: x must be a positive integer.
Sprint-3/2-practice-tdd/repeat.js
Outdated
| function repeat(valueToRepeat, numOfTimes) { | ||
| // Validate count | ||
| if (!Number.isInteger(numOfTimes)) { | ||
| return "Invalid count: count should be an integer"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should ideally have your error message to mention the actual argument that is incorrect. Here you have numOfTimes but say count in the error message. This can be confusing when you try to debug your function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have fixed my code to reflect your correction. Thank you.
| }); | ||
|
|
||
| // case: Object input | ||
| test("should repeat an object count times as a string", () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I am not sure if you want to repeat empty object like this. I don't see value in doing so. I think the requirement of this function is to repeat string n times. If what you pass is not a string, should it perhaps return an error as you did in other functions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I am not sure if you want to repeat empty object like this. I don't see value in doing so. I think the requirement of this function is to repeat string n times. If what you pass is not a string, should it perhaps return an error as you did in other functions?
Hi @jennethydyrova.
I am trying to fix my code. Please, I would like your opinion. Should I let the number input (e.g. 2) repeat or return an error message?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the function requirement is a bit vague, that also stems from an unclear function name (this is about the importance of a good and descriptive fn names!). I would say to keep it simpler, assume that only string should be accepted as an input to be repeated, the rest should throw an error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the function requirement is a bit vague, that also stems from an unclear function name (this is about the importance of a good and descriptive fn names!). I would say to keep it simpler, assume that only string should be accepted as an input to be repeated, the rest should throw an error.
Thank you for your suggestion. I agree that the function name is a bit vague. It would have been clearer if it were something like repeatString instead of just repeat. I will update the function to only allow strings to be repeated and update the tests accordingly.
| typeof stringOfCharacters !== "string" || | ||
| typeof findCharacter !== "string" | ||
| ) { | ||
| return "Invalid input: input should be a string"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's okay to return error message like this as you are just at the beginning of learning js, but there is a better way of doing it. This is optional but can you think of how you can make use of the following https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/throw in the context of error handling in your functions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Member
Hi @jennethydyrova.
I will read through the resource you have provided and update the code. Thank you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don’t need to act on optional comments right now, this one was optional, but feel free to if you’d like!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don’t need to act on optional comments right now, this one was optional, but feel free to if you’d like!
Thank you for clarifying! I’ll keep the optional comments in mind and may try them out later to improve the code further.
Hi @jennethydyrova. |
…: numOfTimes should be an integer"
…ger for numOfTimes; all other inputs return an error message
|
Great work @Iswanna! |
Thank you so much, @jennethydyrova, for the detailed feedback. It's been a great learning experience for me, and I really appreciate it. |
Learners, PR Template
Self checklist
Changelist
This pull request includes modifications to the 2-practice-tdd directory. I added test cases for the various functions in the files, and I also implemented the functions to ensure all tests pass.
Questions
Hi. Please could you review my PR? I’d really appreciate your feedback?