London | 26 - ITP-MAY | Ebrahim Moqbel | Sprint 3 | Implement and ewrite tests - #1586
London | 26 - ITP-MAY | Ebrahim Moqbel | Sprint 3 | Implement and ewrite tests#1586Ebrahim-Moqbel wants to merge 10 commits into
Conversation
…nstead of invalid angle.
There was a problem hiding this comment.
Well done on writing test cases for the border cases.
|
|
||
| function getCardValue(card) { | ||
| // TODO: Implement this function | ||
| let rank = card.slice(0,-1); |
There was a problem hiding this comment.
creating a variable rank and assign the last element which it the face in it.
There was a problem hiding this comment.
I also could use const as I won't be updating the variables latter on.
| function getCardValue(card) { | ||
| // TODO: Implement this function | ||
| let rank = card.slice(0,-1); | ||
| let cardFace = card[card.length - 1]; |
There was a problem hiding this comment.
The indendation of this line is not consistent with the rest of the code. How can you ensure to always format your code consistently?
| if (+rank >= 2 && +rank <= 9) { | ||
| return +rank; | ||
| } |
There was a problem hiding this comment.
converting the string rank to a number.
There was a problem hiding this comment.
This happens 3 times. How could you write the code so the conversion is only done once?
| if (!["♠", "♥", "♦", "♣"].includes(cardFace)) { | ||
| } |
There was a problem hiding this comment.
In code line number 38 a condition check that the elements k,10,Q,J are in the rank, if so will be returning number 10.
in code line number 41 will be matching the card face if any of the elements are not in CardFace will stop everything and throw an error.
| test(`Should throw an error when given an invalid card`, () => { | ||
| expect(() => getCardValue("5X")).toThrow("Invalid card ")}); //invalid suit | ||
| expect(() => getCardValue("1♠")).toThrow("Invalid card "); //invalid rank | ||
| expect(() => getCardValue("3")).toThrow("Invalid card "); //missing suit | ||
| }) |
There was a problem hiding this comment.
Vs code shows me an error here. How can you fix it?
Declaration or statement expected.
There was a problem hiding this comment.
there was a closing curly bracket and parenthese in line 27.
| expect(getCardValue("K♠")).toEqual(10); | ||
| }) | ||
| // Invalid Cards | ||
| test(`Should throw an error when given an invalid card`, () => { |
There was a problem hiding this comment.
What other invalid cases could you test (which other ranks are invalid and what about a missing rank)
There was a problem hiding this comment.
I have added another case in line 27 for testing a missing rank. other invalid cases could be negative ranks.
Luro91
left a comment
There was a problem hiding this comment.
Please note that in CYF courses, the recommended way to inform the reviewer of your changes is to do both of the following:
- Reply to their feedback.
- In the responses, clarify how each piece of feedback was addressed to demonstrate that you've carefully reviewed the suggestions.
- You may find the suggestions in this PR Guide useful.
- Your response may trigger a notification (depending on the reviewer's settings), helping ensure they’re aware of the updates you’ve made.
- In the responses, clarify how each piece of feedback was addressed to demonstrate that you've carefully reviewed the suggestions.
- Replace the "Reviewed" label by a "Needs review" label
- Without this label, the reviewer would not know if your changes is ready to be reviewed.
| if (!["♠", "♥", "♦", "♣"].includes(cardFace)) { | ||
| throw new Error(`Invalid card`); | ||
| } |
There was a problem hiding this comment.
The indendation of this line is not consistent with the rest of the code. How can you ensure to always format your code consistently? What happens if the condition is false?
| if (+rank >= 2 && +rank <= 9) { | ||
| return +rank; | ||
| } |
There was a problem hiding this comment.
This happens 3 times. How could you write the code so the conversion is only done once?
Learners, PR Template
Self checklist
Changelist
completed the implement and rewrite file with fixing a bug
Questions
N/A