LONDON | Pooriya Ketabi | Module-Structuring-and-Testing-Data | Sprint 3 #230
Conversation
Sslim123
added
👀 Review Git
illicitonion
added
the
Review in progress
Sprint-3/implement/get-card-value.js
Outdated
| if (parseInt(rank) >= 2 && parseInt(rank) <= 10) { | ||
| return parseInt(rank); |
There was a problem hiding this comment.
You're calling parseInt on the same input here three times - can you think how you can avoid needing to repeat this? Why is doing the same thing three times not ideal?
There was a problem hiding this comment.
It's not ideal, because of performance and readability (debugging). Therefore it's better to store it in a variable and then use the variable instead.
Thank you for mentioning it.
There was a problem hiding this comment.
These are some really nice tests, good job!
There was a problem hiding this comment.
Thank you. I'm glad you found it nice.
| // ---------------------------------------- Tests: ---------------------------------------- | ||
| // -------- Proper Fraction -------- | ||
| console.assert( | ||
| isProperFraction(2, 3) === true, |
There was a problem hiding this comment.
We actually don't need to put === true or === false in assertions. What we need is an expression which evaluates to a boolean (true or false), and isProperFraction already returns a boolean, so we can remove the === true (and write !isProperFraction(...) where we want to check something is false.
Almost always, writing something === true or something === false should be replaced by just something or !something :)
There was a problem hiding this comment.
I truly appreciate your detailed explanations. Your guidance has been really helpful, and I’ve made the necessary changes accordingly.
There was a problem hiding this comment.
Ideally this directory wouldn't be included in your coursework PR, as this was just your local work during the prep, rather than the exercises you're submitting for review.
There was a problem hiding this comment.
Thank you for pointing that out.
illicitonion
added
Reviewed
PooriyaKTB
added
the
Complete
| let rank = card.slice(0, -1); | ||
| const parseIntRank = parseInt(rank); |
There was a problem hiding this comment.
It's useful to differentiate variable names when you have two things which have the same (but for some reason different, e.g. a different type type) data in them.
I probably wouldn't use parseIntRank as a variable name - parse is a verb, and verbs suggest doing (i.e. function names).
What do you think of each of these variable name pairs?
rankString and rankNumber
rankAsString and rankAsNumber
stringRank and numericRank
rankString and rank
Learners, PR Template
Self checklist
Changelist
Briefly explain your PR.
Questions
Ask any questions you have for your reviewer.