-
-
Notifications
You must be signed in to change notification settings - Fork 239
Glasgow | 25-ITP-SEP | Mohammed Abdoon | Sprint 3 | Coursework/2-practice-tdd #746
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?
Conversation
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.
The function should work for any positive integers.
Can you lookup the rules for representing ordinal numbers?
You should prepare tests in get-ordinal-number.test.js file first before implementing getOrdinalNumber().
For example, we can prepare a test for numbers 2, 22, 132, etc. as
test("append 'nd' to numbers ending in 2, except those ending in 12", () => {
expect( getOrdinalNumber(2) ).toEqual("2nd");
expect( getOrdinalNumber(22) ).toEqual("22nd");
expect( getOrdinalNumber(132) ).toEqual("132nd");
});
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.
Thank you for the feedback. I set the tests for the function and modified the function getOrdinalNumber() to meet the requirements and to handle all possible cases.
Sprint-3/2-practice-tdd/repeat.js
Outdated
| if(count < 0) { | ||
| return "Error: Negative counts are not valid."; | ||
| } |
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.
How would the caller distinguish the result of the following two function calls?
repeat("Error: Negative counts are not valid.", 1)repeat("", -1)
Both function calls return the same value.
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.
Oh yes, the result will be the same, I should have used throw error to handle the invalid inputs.
I modified it to be as follows:
if (typeof count !== "number" || isNaN(count) || count < 0) {
throw new Error("Count must be a valid number");
}
| test("should repeat the string 0 times ( means the output will be an empty string)", () => { | ||
| const str = "hello"; | ||
| const count = -3; | ||
| const repeatedStr = repeat(str, count); | ||
| expect(repeatedStr).toEqual("Error: Negative counts are not valid."); | ||
| }); |
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.
If you modified repeat() to throw an error when count is negative, and you wanted to test if the function can throw an error as expected, you can use .toThrow(). You can find out more about how to use .toThrow() here: https://jestjs.io/docs/expect#tothrowerror (Note: Pay close attention to the syntax of the example)
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 see, I didn't know that I could also test functions using .toThrow. thank you for letting me know, I changed the code so it can check if the function throws an error when passing invalid inputs
|
I made changes to the code and fixed the mistakes. |
cjyuan
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.
Changes look good. I have some follow-ups.
| return "1st"; | ||
| return 0; | ||
| // | ||
| if (typeof num !== "number" || !Number.isInteger(num) || num <= 0) { |
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 condition can be shorten. Can you figure out how?
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.
Yes, I think I one of these conditions typeof num !== "number" || !Number.isInteger(num) is not needed since they both check if the input is a valid number.
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.
typeof num !== "number" and !Number.isInteger(num) are not equivalent. One of them is stricter than the other.
Both Infinity and NaN can still pass this check: typeof num !== "number".
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.
Yes, that's right. They are not equivalent. I tested them and learned that Infinity and NaN are actually type of numbers, that's why the rule typeof num !== "number" won't work here. So the best practice is to use Number.isInteger(), as this method excludes both NaN and Infinity.
I updated the function and added new test cases including NaN, Infinity, strings and arrays and instead of showing a string error message, I used throw an error with an error message
| // Case 3: Identify the ordinal numbers for 3 to 12 | ||
| test("should return correct ordinal suffixes for numbers 3 to 12", () => { | ||
| expect( getOrdinalNumber(3) ).toEqual("3rd"); | ||
| expect( getOrdinalNumber(4) ).toEqual("4th"); | ||
| expect( getOrdinalNumber(5) ).toEqual("5th"); | ||
| expect( getOrdinalNumber(6) ).toEqual("6th"); | ||
| expect( getOrdinalNumber(7) ).toEqual("7th"); | ||
| expect( getOrdinalNumber(8) ).toEqual("8th"); | ||
| expect( getOrdinalNumber(9) ).toEqual("9th"); | ||
| expect( getOrdinalNumber(10) ).toEqual("10th"); | ||
| expect( getOrdinalNumber(11) ).toEqual("11th"); | ||
| expect( getOrdinalNumber(12) ).toEqual("12th"); | ||
| }); | ||
| // Case 4: Identify the ordinal numbers for numbers above 12 | ||
| test("should return correct ordinal suffixes for numbers above 12", () => { | ||
| expect( getOrdinalNumber(13) ).toEqual("13th"); | ||
| expect( getOrdinalNumber(19) ).toEqual("19th"); | ||
| expect( getOrdinalNumber(23) ).toEqual("23rd"); | ||
| expect( getOrdinalNumber(34) ).toEqual("34th"); | ||
| expect( getOrdinalNumber(45) ).toEqual("45th"); | ||
| expect( getOrdinalNumber(56) ).toEqual("56th"); | ||
| expect( getOrdinalNumber(67) ).toEqual("67th"); | ||
| }); | ||
|
|
||
| // Case 5: Identify the ordinal numbers for numbers like 20 30 40 etc | ||
| test("should return correct ordinal suffixes for multiples of ten", () => { | ||
| expect( getOrdinalNumber(20) ).toEqual("20th"); | ||
| expect( getOrdinalNumber(30) ).toEqual("30th"); | ||
| expect( getOrdinalNumber(70) ).toEqual("70th"); | ||
| expect( getOrdinalNumber(50) ).toEqual("50th"); | ||
| }); | ||
|
|
||
| // Case 6: dealing with big numbers | ||
| test("should return correct ordinal suffixes for big numbers", () => { | ||
| expect( getOrdinalNumber(111) ).toEqual("111th"); | ||
| expect( getOrdinalNumber(4712) ).toEqual("4712th"); | ||
| expect( getOrdinalNumber(10003) ).toEqual("10003rd"); | ||
| expect( getOrdinalNumber(10012) ).toEqual("10012th"); | ||
| }); |
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.
Why categorize the input number into 3-12, and >12? Why choose 12 as the cutting point?
Why do multiples of 10 need a separate category?
They way the possible input values are being categorized seem a bit illogical.
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.
The categories (3–12, >12, multiples of 10) are not part of the logic itself, they’re just for organizing the test cases.
I grouped them that way to make sure the function covered different patterns
3 - 10 is for testing numbers that only consist of 1 number, maybe I should have categorized them from 0 to 9.
11 and 12 are special exceptions (11th, 12th), when the last number is 1 or 2 it will always be "st" and "nd" except for these cases.
Multiples of 10 are a separate group just to confirm that round numbers still get “th”.
It’s not about logic segmentation, just clearer test coverage.
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.
Ideally, the test description should help a developer determine which cases the function needs to handle.
When a test fails, the description should also help the developer identify which case the function missed.
While your categories do cover all numbers, here’s another way to categorize the input (for your reference):
- Numbers that end with "st"
- Numbers that end with "nd"
- Numbers that end with "rd"
- Numbers that end with "th"
- Numbers whose last two digits are in the range 11–13 (special cases)
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.
Thanks a lot for the explanation and for clearing things up! I’ve re-categorized the test cases like you suggested.
Sprint-3/2-practice-tdd/repeat.js
Outdated
| function repeat(str, count) { | ||
| if(count < 0) { | ||
| return "Error: Negative counts are not valid."; | ||
| if (typeof count !== "number" || isNaN(count) || count < 0) { |
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.
Can count be 0.5 or Infinity?
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.
if count is 0.5, the function will treat it as 0, so an empty string will be returned.
If count is Infinity, the function will throw a RangeError because a string cannot be repeated infinitely.
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.
Why define a condition that rejects NaN but not Infinity?
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 didn't know what really the difference was. but I changed the rule and used Number.isInteger() instead of isNaN.
Number.isInteger() is more general and can detect both NaN and Infinity.
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.
If you have time, look up the characteristics of "number" type values in JS, what are some of the edge cases, and how to check them.
When writing programs, it is important to know the limits of each type of values.
|
Thank you for the feedback. Changes have been made. |
|
Changes have been made. |
|
Changes look good. Well done. |
Learners, PR Template
Self checklist
Changelist
I modified the function countChar() so it can find a character withing a string and returns its occurrence times if it exists.
also the function getOrdinalNumber() to return "1st" when the input is 1 or 0 when the input number is not 1.
also modified the function repeat() to repeat the str count times, and returns string error if count is negative.
the test files are also updated with the new test cases.
Questions
No questions.