London | 26-ITP-Jan | Oussama Mouggal | sprint 3 | implement-and-rewrite-tests#1026
Conversation
…ts for all card types
Oussama-Mouggal
added
the
Needs Review
Oussama-Mouggal
changed the title
LonMcGregor
added
the
Review in progress
LonMcGregor
left a comment
LonMcGregor
left a comment
There was a problem hiding this comment.
Very good work adding lots of test cases - especially catching edge cases.
You may want to take a closer look at some of the implementation which could be improved
| console.error("Error was not thrown for extra characters"); | ||
| } catch (e) {} | ||
|
|
||
| console.log("All tests completed!"); |
There was a problem hiding this comment.
This code is imported as a module for another test. Be careful about putting extra console.logs here.
There was a problem hiding this comment.
I made the required changes to run the code only when the file is executed directly.Thanks
|
|
||
| function isProperFraction(numerator, denominator) { | ||
| // TODO: Implement this function | ||
| return numerator > 0 && denominator > 0 && numerator < denominator; |
There was a problem hiding this comment.
0 / 5 is a valid proper fraction. Can you take another look at the tests and implementation for this?
There was a problem hiding this comment.
Thank you for your feedback, I updated isProperFraction to fit all required test cases.
| const rank = card.slice(0, -1); | ||
|
|
||
| // Validate suit | ||
| if (!validSuits.includes(suit)) { |
There was a problem hiding this comment.
You have two if branches that end up doing the same thing. Is there a way they could be combined?
There was a problem hiding this comment.
Thank you @LonMcGregor for your valuable feedback. I combined the two branches with || condition for a cleaner code
LonMcGregor
added
Reviewed
Oussama-Mouggal
added
the
Needs Review
2 participants
Self checklist
Changelist
I rewrited the code and tested edge cases using jest