West Midlands | 25 Sep ITP | Iswat Bello | Sprint 3 | Implement and rewrite tests #800
Conversation
…hrow an error for invalid cards
Iswanna
added
📅 Sprint 3
| if (numerator < denominator && numerator !== 0) { | ||
| return true; | ||
| } else if (numerator >= denominator) { | ||
| return false; | ||
| } else if (numerator === 0) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
The function won't work properly if numerator and denominator are allowed to be negatives.
For example -3 < 2 but -3/2 is not a proper fraction.
| function getCardValue(card) { | ||
| if (rank === "A") { | ||
| card = card.substring(0 , card.length -1); // Remove the suit emoji | ||
| console.log(card); |
There was a problem hiding this comment.
For best practices, we should ensure code submitted in a PR free of any debugging code.
| return 11; | ||
| } if (card === "J" || card === "Q" || card === "K" || card === "10") { | ||
| return 10; | ||
| } else if (card >= 2 && card <=9) { |
There was a problem hiding this comment.
In JavaScript, strings that represent valid numeric literals in the language can be safely
converted to equivalent numbers. Do you want to recognize these string values as valid ranks?
To find out what these strings are, try asking AI
What kinds of string values would make
card >= 2 && card <=9evaluate to true?
| // just make one change at a time -- don't rush -- programmers are deep and careful thinkers | ||
| function getCardValue(card) { | ||
| if (rank === "A") { | ||
| card = card.substring(0 , card.length -1); // Remove the suit emoji |
There was a problem hiding this comment.
Why not use a separate variable rank to represents the rank of a card?
We could reuse a variable but it is better not to use the same variable to store different kinds of value.
- "A♠" => represent the value of a card
- "A" ==> represent the value of a rank
| test("should identify reflex angle (>180° and <360°)", () => { | ||
| expect(getAngleType(250)).toEqual("Reflex angle"); | ||
| }); |
There was a problem hiding this comment.
To make a test more robust, we could specify multiple expect(...) statements within each test() to cover multiple values that belong to the same case. For example,
test("should identify reflex angle (>180° and <360°)", () => {
expect(getAngleType(300)).toEqual("Reflex angle");
expect(getAngleType(359.999)).toEqual("Reflex angle");
expect(getAngleType(180.001)).toEqual("Reflex angle");
});
| test("should return false for negative fractions", () => { | ||
| expect(isProperFraction(-4, 9)).toEqual(true); | ||
| }) |
There was a problem hiding this comment.
The test description does not quite match the test being performed.
| test("should return the numeric value for number cards from 2 to 10", () => { | ||
| const numberCard = getCardValue("7♥"); | ||
| expect(numberCard).toEqual(7); | ||
| }) |
There was a problem hiding this comment.
Could consider testing multiple values, especially the boundary cases "2♥" and "10♥".
cjyuan
added
Reviewed
|
Hi @cjyuan. Thank you so much for all the feedback. I will work on all of them and update all the necessary files. |
…s; update function to handle them
2 participants
Learners, PR Template
Self checklist
Changelist
This pull request involves modifications to two directories:
Implement directory: I added assertions using
console.assertand built the program case by case.Rewrite-tests-with-Jest directory: I rewrote the existing test cases using Jest and ran
npm testto verify the implementation of the getCardValue function.Questions
Hi. Please could you review my PR? I’d really appreciate your feedback?