London | May-2026-itp | Vitalii Kmit | Sprint 3 | Implement and rewrite tests#1463
London | May-2026-itp | Vitalii Kmit | Sprint 3 | Implement and rewrite tests#1463Vitalii-code wants to merge 1 commit into
Conversation
Vitalii-code
added
the
Needs Review
illicitonion
added
Review in progress
illicitonion
left a comment
illicitonion
left a comment
There was a problem hiding this comment.
This generally looks good, but I left a few comments to look at :) Thanks!
| // TODO: Implement this function | ||
| if (angle > 0 && angle < 90) { | ||
| return "Acute angle"; | ||
| } else if (angle == 90) { |
There was a problem hiding this comment.
Here you're using == but JavaScript also has a === operator. Do you know the difference? Why have you chosen to use == here? When would === make more sense?
This applies in many places in your PR - please take a look at them all!
| return "Right angle"; | ||
| } else if (angle > 90 && angle < 180) { | ||
| return "Obtuse angle"; | ||
| } else if (angle == 180) { |
|
|
||
| function getCardValue(card) { | ||
| // TODO: Implement this function | ||
| const lastChar = card.length - 1; |
There was a problem hiding this comment.
The name of this variable suggests it contains a character. But it actually contains an index of a character. Can you think of a name that could make this more clear?
| } | ||
|
|
||
| if (!(suit === "♠" || suit === "♥" || suit === "♦" || suit === "♣")) { | ||
| throw new Error("Invalid suit ", suit); |
There was a problem hiding this comment.
It's often useful to include in an error message what the invalid data was. Imagine you're are running a big long programme, and aren't sure exactly what data is being processed - the more context we can give in errors the better.
| const rank = card.slice(0, lastChar); | ||
| const suit = card.slice(lastChar); | ||
|
|
||
| if (rank === "10") { |
There was a problem hiding this comment.
This works, but I'm curious - why did you decide to handle 10 specially here?
What would happen if someone passed the string "10Q" to this function? What would you want to happen? What other edge cases may you want to test for in your tests?
There was a problem hiding this comment.
Please revert the changes to this file, they're not relevant for this exercise.
illicitonion
added
Reviewed
2 participants
Learners, PR Template
Self checklist
Changelist
In this PR I worked on adding tests with plain javascript and then with jest