Repository navigation
London | 26-ITP-Sep | Sakiya Mayow | Sprint 2 | Implement and rewrite tests - #1766
zakiaao-tech wants to merge 10 commits into
Conversation
zakiaao-tech
added
the
Needs Review
hackertainment
added
Review in progress
hackertainment
left a comment
hackertainment
left a comment
There was a problem hiding this comment.
The code generally looks ok to me. Please keep it up.
| test(`should return "Invalid angle" when ( angle <= 0 or angle >= 360)`, () => { | ||
| expect(getAngleType(181)).toEqual("Reflex angle"); | ||
| expect(getAngleType(270)).toEqual("Reflex angle"); | ||
| expect(getAngleType(359)).toEqual("Reflex angle"); | ||
| }); |
There was a problem hiding this comment.
This seems like the duplicate of the Case 5 tests above. Please fix it, thank you.
| // Case 5: Negative numerator | ||
| test("should return true when numerator is negative and denominator is positive", () => { | ||
| assert.equal(isProperFraction(-1, 2), true); | ||
| }); | ||
|
|
||
| // Case 6: Negative denominator | ||
| test("should return false when denominator is negative", () => { | ||
| assert.equal(isProperFraction(1, -2), false); | ||
| }); |
There was a problem hiding this comment.
Although there are different definitions for negative proper fractions (one definition returns true and another returns false) and I would treat both definitions as correct solution, these two cases should be consistently returned the same result regardless of which definition to follow. So please fix the test cases (and the program if necessary).
Also, how about both numerator and denominator are negative numbers (e.g. -1/-2 and -2/-1) ? Please add them to the test suite as well. Thank you.
| test("should return true when numerator is zero", () => { | ||
| expect(isProperFraction(0, 2)).toBe(true); | ||
| }); | ||
|
|
||
| test("should return false when denominator is zero", () => { | ||
| expect(isProperFraction(1, 0)).toBe(false); |
There was a problem hiding this comment.
If only considering positive proper fraction, these two cases should both return false. Depends on how you fix https://github.com/CodeYourFuture/Module-Structuring-and-Testing-Data/pull/1766/changes#r4423374448 , one of these test cases should be fixed as well. Thank you.
|
|
||
| export function getCardValue(card) { | ||
| // TODO: Implement this function | ||
| if (typeof card !== "string" || !/^(A|[2-9]|10|J|Q|K)[♠♥♦♣]$/.test(card)) { |
There was a problem hiding this comment.
Your program is correct, and no need to change anything. Just I would like to know more about your program design process:
- Can you explain the breakdown of
!/^(A|[2-9]|10|J|Q|K)[♠♥♦♣]$/.test(card)expression? - What is the advantages of this approach over other implementations?
Thank you for adding a reply to this comment.
hackertainment
added
Reviewed
Learners, PR Template
Self checklist
Task code
CYF-1059.
Changelist
Completed the sprint 2 exercises, including implementing functions and writing and rewriting tests using TDD and Jest.