London | 26-ITP-May | Dipa Sarker | Sprint 3 | Implement and rewrite final#1470
Conversation
|
Hello Reviewers, |
Dipa-Sarker
added
the
Needs Review
| if (angle <= 0 || angle >= 360) { | ||
| return "Invalid angle"; | ||
| } | ||
| else if (angle == 90) { |
There was a problem hiding this comment.
Whats the difference between == and === and why are you using == here?
| //module.exports = getAngleType; | ||
|
|
||
| // This helper function is written to make our assertions easier to read. | ||
| // If the actual output matches the target output, the test will pass | ||
| // function assertEquals(actualOutput, targetOutput) { | ||
| //console.assert( | ||
| //actualOutput === targetOutput, | ||
| //`Expected ${actualOutput} to equal ${targetOutput}` | ||
| //); | ||
| //} | ||
|
|
||
| // TODO: Write tests to cover all cases, including boundary and invalid cases. | ||
| // Example: Identify Right Angles | ||
| //const right = getAngleType(90); | ||
| //assertEquals(right, "Right angle"); No newline at end of file |
| expect(getAngleType(91)).toEqual("Obtuse angle"); | ||
| expect(getAngleType(120)).toEqual("Obtuse angle"); | ||
| expect(getAngleType(179)).toEqual("Obtuse angle"); | ||
| }); | ||
| test(`should return "Straight angle" when (0 == 180)`, () => { | ||
| expect(getAngleType(180)).toEqual("Straight angle"); | ||
| }); | ||
| test(`should return "Reflex angle" when (180 < 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.
Good job on testing the border cases here
| expect(getAngleType(-1)).toEqual("Invalid angle"); | ||
| expect(getAngleType(370)).toEqual("Invalid angle"); |
There was a problem hiding this comment.
Which border cases can you test here? (Which numbers are closest to the condition?)
| test(`should return false when numerator is negative`, () => { | ||
| expect(isProperFraction(-1, 1)).toEqual(false); | ||
| }); | ||
| test(`should return false when denominator is negative`, () => { | ||
| expect(isProperFraction(1, -1)).toEqual(false); | ||
| }); No newline at end of file |
There was a problem hiding this comment.
What about values like -1/2 or 1/-2. What should the function return for them?
|
|
||
| function isProperFraction(numerator, denominator) { | ||
| // TODO: Implement this function | ||
| if (numerator <= 0 || denominator <= 0) { |
There was a problem hiding this comment.
Why does the numerator be bigger than 0?
| if (!validRank.includes(rank) || !validSuit.includes(suit)) { | ||
| throw new Error("Invalid Card"); | ||
| } | ||
|
|
||
| if (rank === "A") { | ||
| return 11; | ||
| } | ||
| else if (rank === "J" || rank=== "Q" || rank === "K") { | ||
| return 10; | ||
| } | ||
|
|
||
| // The line below allows us to load the getCardValue function into tests in other files. | ||
| // This will be useful in the "rewrite tests with jest" step. | ||
| module.exports = getCardValue; | ||
| else if (rank >= 2 && rank <= 10) { | ||
| return Number(rank); | ||
| } |
There was a problem hiding this comment.
The indentation is a bit off. How can you ensure consistent formatting in your code?
| const validRank = ["A","2","3","4","5","6","7","8","9","10","J","Q","K"]; | ||
| const validSuit = ["♠","♥","♦","♣"]; |
There was a problem hiding this comment.
Good job on explicitly listing all allowed ranks and suits. This makes the code easier to read and the logic for checking easier as well.
| // The line below allows us to load the getCardValue function into tests in other files. | ||
| // This will be useful in the "rewrite tests with jest" step. | ||
| module.exports = getCardValue; | ||
| else if (rank >= 2 && rank <= 10) { |
There was a problem hiding this comment.
What happens if the condition is false?
| expect(getCardValue("K♦")).toEqual(10); | ||
| }); | ||
| }); | ||
| describe("Invalid Cards", () => { |
There was a problem hiding this comment.
Which part of the condition is not covered by the invalid test cases yet?
if (!validRank.includes(rank) || !validSuit.includes(suit)) {
Luro91
added
Reviewed
2 participants
Learners, PR Template
Self checklist
Changelist