London| 25-ITP-Sep | Sophia Mohamed| Sprint 3|Coursework/sprint-3-implement-and-rewrite #839
Conversation
London | 25-ITP-Sept | Sophia Mohamed | Sprint 1 | coursework/sprint-1
saff-coder
added
the
Needs Review
jennethydyrova
added
Review in progress
jennethydyrova
left a comment
jennethydyrova
left a comment
There was a problem hiding this comment.
Hi @saff-coder! Really great job on this PR. I left couple of comments that need to be addressed but overall, very high quality work.
Can you please remove unrelated files from this PR? You should commit only relevant files to keep PR clean.
| return "Reflex angle"; | ||
| } | ||
| else { | ||
| return "Invalid angle"; // Optional: handles 0 or ≥ 360 |
There was a problem hiding this comment.
This comment is a bit misleading. Is this optional or default? Those are two different things.
There was a problem hiding this comment.
I have removed line 32, there was no need to add an Invalid angle
There was a problem hiding this comment.
You should keep it, it's a nice default case or it could be a good if condition to catch invalid values for angle parameter. Either way, you should have one of them. Think of it this way, if your angle value is 500 or -1, how will your function behave?
| const reflex = getAngleType(270); | ||
| assertEquals(reflex, "Reflex angle"); | ||
|
|
||
| //console.assert(reflex === "Reflex angle", `Expected ${reflex} to equal Reflex angle`); |
There was a problem hiding this comment.
Let's remove any scratch work that doesn't contribute to PR to keep it clean and focused.
| // What other scenarios could you test for? | ||
| const negativeDenominator = isProperFraction(3, -5); | ||
| assertEquals(negativeDenominator, true); | ||
| const bothNegative = isProperFraction(-2, -6); |
There was a problem hiding this comment.
looks like this one is missing assert
| if (rank === "A") { | ||
| return 11; | ||
| } | ||
| if (["J", "Q", "K", "10"].includes(rank)) { |
There was a problem hiding this comment.
This is correct but you could have this block as else if because it's a continuation of the same logic. Something to note for future
| expect(getCardValue("9♥")).toEqual(9); | ||
| }); | ||
| // Case 3: Handle Face Cards (J, Q, K): | ||
| // Case 4: Handle Ace (A): |
There was a problem hiding this comment.
I think you are missing this test case. Could you please add it?
jennethydyrova
added
Reviewed
saff-coder
added
the
Needs Review
|
Looks good to me @sofayas, nice work! As a side note, can you please install prettier and make sure it formats your code correctly from next PR onward? |
jennethydyrova
added
Complete
|
Thank you Jennet, for all your help and advice |
2 participants
Learners, PR Template
Self checklist
Changelist
I have completed tasks as required.