-
-
Notifications
You must be signed in to change notification settings - Fork 261
London | 25-ITP-Sep | Adnaan Abo | Sprint 3 | coursework/sprint-3-implement-and-rewrite #713
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 11 commits
3cbfc96
61f0da7
f6e6f38
6fe6458
b07437b
6152bda
d8e8c60
48c7af8
7f195fc
db40788
91ced4e
991180d
d6b9753
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -8,9 +8,20 @@ | |||||
| // write one test at a time, and make it pass, build your solution up methodically | ||||||
| // just make one change at a time -- don't rush -- programmers are deep and careful thinkers | ||||||
| function getCardValue(card) { | ||||||
|
|
||||||
| const rank = card.slice(0, -1); // Extract rank before the suit | ||||||
|
|
||||||
| if (rank === "A") { | ||||||
| return 11; | ||||||
| } else if (["K", "Q", "J", "10"].includes(rank)) { | ||||||
| return 10; | ||||||
| } else if (!isNaN(rank) && Number(rank) >= 2 && Number(rank) <= 9) { | ||||||
| return Number(rank); | ||||||
| } else { | ||||||
| throw new Error('Invalid card rank: "${rank}"'); //rather than just "Invalid card rank" we can show the actual rank that was invalid | ||||||
| } | ||||||
|
|
||||||
|
|
||||||
| } | ||||||
|
|
||||||
| // The line below allows us to load the getCardValue function into tests in other files. | ||||||
|
|
@@ -40,18 +51,41 @@ assertEquals(aceofSpades, 11); | |||||
| // Then it should return the numeric value corresponding to the rank (e.g., "5" should return 5). | ||||||
| const fiveofHearts = getCardValue("5♥"); | ||||||
| // ====> write your test here, and then add a line to pass the test in the function above | ||||||
| assertEquals(fiveofHearts, 5); | ||||||
|
|
||||||
| // Handle Face Cards (J, Q, K): | ||||||
| // Given a card with a rank of "10," "J," "Q," or "K", | ||||||
| // When the function is called with such a card, | ||||||
| // Then it should return the value 10, as these cards are worth 10 points each in blackjack. | ||||||
| const kingofDiamonds = getCardValue("K♦"); | ||||||
| assertEquals(kingofDiamonds, 10); | ||||||
| const jackofClubs = getCardValue("J♣"); | ||||||
| assertEquals(jackofClubs, 10); | ||||||
| const queenofHearts = getCardValue("Q♥"); | ||||||
| assertEquals(queenofHearts, 10); | ||||||
| const tenofSpades = getCardValue("10♠"); | ||||||
| assertEquals(tenofSpades, 10); | ||||||
|
|
||||||
| // Handle Ace (A): | ||||||
| // Given a card with a rank of "A", | ||||||
| // When the function is called with an Ace, | ||||||
| // Then it should, by default, assume the Ace is worth 11 points, which is a common rule in blackjack. | ||||||
| const aceofClubs = getCardValue("A♣"); | ||||||
| assertEquals(aceofClubs, 11); | ||||||
|
|
||||||
| // Handle Invalid Cards: | ||||||
| // Given a card with an invalid rank (neither a number nor a recognized face card), | ||||||
| // When the function is called with such a card, | ||||||
| // Then it should throw an error indicating "Invalid card rank." | ||||||
| const invalidCard = () => getCardValue("1♠"); | ||||||
| // ====> write your test here, and then add a line to pass the test in the function above | ||||||
| try { | ||||||
illicitonion marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
| invalidCard(); | ||||||
| console.assert(false, "Expected an error to be thrown for invalid card rank"); | ||||||
| } catch (e) { | ||||||
| console.assert( | ||||||
| e.message === 'Invalid card rank: "${rank}"', | ||||||
|
||||||
| e.message === 'Invalid card rank: "${rank}"', | |
| e.message === 'Invalid card rank: "1"', |
I think you'll find that the test is actually failing, so we should fix the code :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank for the feedback and I have now included "1" in the expected error message. I have run the test with a pass.
thank you for the suggested change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just ran the test and it failed...
% node Sprint-3/1-implement-and-rewrite-tests/implement/3-get-card-value.js
Assertion failed: Expected error message to be "Invalid card rank" but got "Invalid card rank: "${rank}""Can you check again?
(Also, your Jest tests wouldn't catch this same bug, can you fix them up so they do as well?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for picking on that and being patient with me. test was not failing on my side because Jest’s .toThrow("Invalid card rank") only checks that the error message contains that substring, not that it matches exactly. now I have updated the code and it passes as it should.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could have handled "10" in either the K/Q/J case, or the 2-9 case - which do you prefer? What advantages/disadvantages do you think each has?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hello,
Thank you for the feedback, I prefer to handle the "10" in the K/Q/J case. for clarity and explicitness, and to maintain semantical grouping since "10" has the same card value as the face cards. a disadvantage could be mixing string handling logic "K/Q/J" with numeric checking logic "10".
We can also handle "10" in the 2-9 case and the pros for this would be logical consistency the numeric branch handles anything numeric between 2-10. and its easier to extend if we ever want to adjust the ranges to include 1 or 11 or validate different ranges we just change the numeric branch. Disadvantages risk of mis-parsing since !isNaN(rank) is loose we might accidently accept strings like "10abc". Edge-case handling we need to make sure to handle trimming, ect. in case someone passes weird strings like "010" and " 10 ".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good, thanks for sharing your thoughts!
Personally I prefer 10 to be included in the number case, because it leads to simpler code. In the numeric case, we already need an upper bound, so the difference between
Number(rank) <= 9andNumber(rank) <= 10is really small, whereas adding a whole extra element to the array we're checking is adding an extra piece:["K", "Q", "J", "10"]vs["K", "Q", "J"].But you're right that there's not one obviously better answer - there are advantages and disadvantages to each :)