London | 26-ITP-May | Damilola Odumosu| Sprint 3 | coursework#1528
London | 26-ITP-May | Damilola Odumosu| Sprint 3 | coursework#1528d-odumosu wants to merge 10 commits into
Conversation
d-odumosu
added
📅 Sprint 3
cjyuan
added
Review in progress
| if (Number.isFinite(numerator) && Number.isFinite(denominator)) { | ||
| if(numerator < denominator){ | ||
| return true | ||
| } | ||
| } return false |
There was a problem hiding this comment.
- What are the expected value of these function calls?
isProperFraction(-1, 0)
isProperFraction(-1, -5);
-
What authoritative source do you base your definition of a proper fraction on?
-
Code is not consistently formatted.
Have you installed the prettier VSCode extension and enabled "Format on save/paste" on VSCode,
as recommended in https://github.com/CodeYourFuture/Module-Structuring-and-Testing-Data/blob/main/readme.md ?
| const rank = card.slice(0, card.length - 1).toUpperCase() | ||
| const suit = card.slice(card.length - 1) |
There was a problem hiding this comment.
.slice() accepts negative index. card.slice(card.length - 1) is identical to card.slice(-1).
|
|
||
| // 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?
| 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.
cjyuan
added
Reviewed
2 participants
Self checklist
Changelist
Completed implementing and rewriting tests