London | 26-ITP-May | Damilola Odumosu| Sprint 3 | coursework - #1528
London | 26-ITP-May | Damilola Odumosu| Sprint 3 | coursework#1528d-odumosu wants to merge 19 commits into
Conversation
d-odumosu
added
📅 Sprint 3
cjyuan
added
Review in progress
|
|
||
| // Suit and rank validation | ||
| if (!validSuits.includes(suit)) { | ||
| throw new Error("Invalid card played, suit is missing"); |
There was a problem hiding this comment.
The suit character may not be "missing". For examples, card could be "3♡" (instead of "3♥") or "3♥ ".
Could you think of a more general error message?
| if (card === "") { | ||
| throw new Error("No card was played") | ||
| } | ||
| if (card.length < 2 || card.length > 3) { | ||
| throw new Error("Invalid card played, rank and suit cannot be less than 1 or more than 3") | ||
| } |
There was a problem hiding this comment.
What do "played" and "suit cannot be less than 1 or more than 3" mean?
Are these checks necessary?
There was a problem hiding this comment.
I am validating the length of the card played, i see it might be redundant since i already have suit and rank validation, i will remove the code
| expect(getAngleType(359)).toEqual("Reflex angle"); | ||
| }) | ||
| // Case 6: Invalid angles | ||
| test(`should return "Invalid angle" when (0 > angle > 360)`, () => { |
There was a problem hiding this comment.
The notation 0 > angle > 360 tends to suggests angle is between two values. Could you change the notation?
| // Special case: numerator is zero | ||
| test(`should return false when denominator is zero`, () => { | ||
| expect(isProperFraction(1, 0)).toEqual(false); | ||
| test("should correctly identify proper fractions", () => { |
There was a problem hiding this comment.
Could probably break this test category into several more specific test categories.
There was a problem hiding this comment.
reimplemented the tests, thank you
cjyuan
added
Reviewed
| test(`should return "Invalid angle" when angle is less than 0 or greater than 360')`, () => { | ||
| // Test various acute angles, including boundary cases | ||
| expect(getAngleType(1)).toEqual("Acute angle"); | ||
| expect(getAngleType(45)).toEqual("Acute angle"); | ||
| expect(getAngleType(89)).toEqual("Acute angle"); | ||
| }); |
There was a problem hiding this comment.
This does not look correct.
| if (!validSuits.includes(suit)) { | ||
| throw new Error("Invalid card: suit is not recognised"); | ||
| } | ||
| if (validRanks.includes(rank)) { | ||
| if (rank === "A") { | ||
| return 11; | ||
| } else if (rank === "J" || rank === "Q" || rank === "K") { | ||
| return 10; | ||
| } else { | ||
| throw new Error("Invalid rank"); | ||
| return Number(rank); | ||
| } | ||
|
|
||
| } else { | ||
| throw new Error("Invalid rank"); | ||
| } |
There was a problem hiding this comment.
Could consider organizing the code in this manner (esp. when the invalid cases can be determined easily):
// Code to check for all invalid cases
if ( ... ) throw ...
if ( ... ) throw ...
// Code to deal only with valid values
There was a problem hiding this comment.
all changes have been made, thank you
| }); | ||
| // Case 6: Invalid angles | ||
| test(`should return "Invalid angle" when (0 > angle > 360)`, () => { | ||
| test(`should return "Invalid angle" when angle is less than zero or angle is greater than 360)`, () => { |
There was a problem hiding this comment.
This test description is still not quite correct.
"Greater than" means >, which is not the same as >=.
2 participants
Self checklist
Changelist
Completed implementing and rewriting tests