-
-
Notifications
You must be signed in to change notification settings - Fork 240
London | 25-ITP-May | Houssam Lahlah | Sprint 2 | Acoursework/sprint 3 #699
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
Conversation
…dle, and last names.
…om file path string using slice.
…ween minimum and maximum.
Added comments to exclude instructional lines from execution.
Updated age variable to let to allow incrementing its value
Declare variable before using in template string.
Add comments explaining slice error and fix by converting number to string
…umerator cases and add stretch scenarios for isProperFraction edge case testing
…id cards and throw error for invalid ones
…e cards; throw error for invalid cards
- Return proper suffixes for 1st, 2nd, 3rd, and others (e.g., 4th, 11th, 23rd) - Added Jest tests for various edge cases including teens (11th-13th) - Improved function to handle general inputs dynamically
- Added str and count parameters to repeat function - Used String.repeat to repeat str count times - Throws error if count is negative - Added tests for count = 1, 0, and negative values
…iled comments and explanations
| const number = Number(rank); | ||
| if (number >= 2 && number <= 9) return 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.
Does your function return the value you expected from each of the following function calls?
getCardValue("0x02♠");
getCardValue("2.1♠");
getCardValue("0002♠");
| test("should return 5 for Five of Hearts", () => { | ||
| expect(getCardValue("5♥")).toEqual(5); | ||
| }); | ||
|
|
||
| test("should return 10 for Ten of Diamonds", () => { | ||
| expect(getCardValue("10♦")).toEqual(10); | ||
| }); |
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.
When preparing tests, we should ensure the tests cover all possible cases. If we specify a test for individual card, we will need about 53 tests to cover all possible cases. Instead, we could consider classifying all possible values into different categories, and then within each category we test some samples.
For example, one possible category for getCardValue() is, "should return the value of number cards (2-10)", and we can prepare the test as
test("should return the value of number cards (2-10)", () => {
expect(getCardValue("2♣︎")).toEqual(2);
expect(getCardValue("5♠")).toEqual(5);
expect(getCardValue("10♥")).toEqual(10);
// Note: We could also use a loop to check all values from 2 to 10.
});
| test("should return 10 for Jack of Clubs", () => { | ||
| expect(getCardValue("J♣")).toEqual(10); | ||
| }); | ||
|
|
||
| test("should return 10 for Queen of Spades", () => { | ||
| expect(getCardValue("Q♠")).toEqual(10); | ||
| }); | ||
|
|
||
| test("should return 10 for King of Hearts", () => { | ||
| expect(getCardValue("K♥")).toEqual(10); | ||
| }); |
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.
We could generalise this test to "should return 10 for face cards (J, Q, K)" and check all three ranks J, Q, K).
|
|
||
| test("should return '2nd' for 2", () => { | ||
| expect(getOrdinalNumber(2)).toEqual("2nd"); | ||
| }); | ||
|
|
||
| test("should return '3rd' for 3", () => { | ||
| expect(getOrdinalNumber(3)).toEqual("3rd"); | ||
| }); | ||
|
|
||
| test("should return '4th' for 4", () => { | ||
| expect(getOrdinalNumber(4)).toEqual("4th"); | ||
| }); | ||
|
|
||
| test("should return '11th' for 11", () => { | ||
| expect(getOrdinalNumber(11)).toEqual("11th"); | ||
| }); |
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 tests are not comprehensive; they do not quite cover all possible cases.
To ensure thorough testing, we need broad scenario coverage. Listing individual values, however, can quickly lead to an unmanageable number of test cases.
Instead of writing tests for individual numbers, consider grouping all possible input values into meaningful categories. Then, select representative samples from each category to test. This approach improves coverage and makes our tests easier to maintain.
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.
Because of a bug, I don't think this script can be executed successfully. Can you try executing this file and fix the error?
In addition, it is a good practice to delete unused code when submitting the code for review.
Self checklist
Changelist
This pull request for syn my fork and updating my branch.
Questions
Please, Let me know if there is anything else went wrong?