Manchester | 26-ITP-May | Yee Man Tsang | Sprint 3 | 1-implement-and-rewrite-tests#1496
Manchester | 26-ITP-May | Yee Man Tsang | Sprint 3 | 1-implement-and-rewrite-tests#1496lintsang wants to merge 2 commits into
Conversation
This comment has been minimized.
This comment has been minimized.
lintsang
added
the
Needs Review
This comment has been minimized.
This comment has been minimized.
github-actions
Bot
removed
the
Needs Review
lintsang
added
the
Needs Review
Liam310
added
Review in progress
| return "Right angle"; | ||
| }else if (angle < 180){ | ||
| return "Obtuse angle"; | ||
| }else if (angle == 180){ |
There was a problem hiding this comment.
What's the difference between == and ===? Which one do you think is more appropriate here?
There was a problem hiding this comment.
I learnt the difference between == and ===. Here we are checking a number value so === is needed here.
| return true; | ||
| }else{ | ||
| return false; | ||
| } |
There was a problem hiding this comment.
I think there's a nice refactor we could do here... given that numerator < denominator already evaluates to true or false, how might you refactor this solution to not need an if/else statement and just be a single return statement?
There was a problem hiding this comment.
Thanks! I will remember this refractor code.
| const suit = card.slice(-1); | ||
| //console.log(`number is ${number}`); | ||
| //console.log(typeof suit); | ||
| //console.log(suit); |
There was a problem hiding this comment.
Perfectly valid to use console.log to figure out the problem as we're solving it, but make sure to remove any unnecessary logs and/or comments like these for the final solution 🙂
| console.log("Error thrown for invalid card 🎉"); | ||
| } | ||
|
|
||
| // What other invalid card cases can you think of? |
There was a problem hiding this comment.
Like the comment says - what other invalid card cases can you think?
There was a problem hiding this comment.
Hint: Think of an input that has the right format but isn't valid!
There was a problem hiding this comment.
I tried to think about anything I may miss and think maybe if the input is a number instead if string. Hope this is what you are asking to check
| }); | ||
|
|
||
| // Case 4: Invalid card | ||
| test(`Should return 10 when given an ace card`, () => { |
There was a problem hiding this comment.
I'm not sure this test description matches what the test is checking for!
There was a problem hiding this comment.
Sorry, careless mistake is udpated.
| expect(getCardValue("4")).toEqual("invalid"); | ||
| expect(getCardValue("")).toEqual("invalid"); | ||
| expect(getCardValue("m")).toEqual("invalid"); | ||
| expect(getCardValue("300")).toEqual("invalid"); |
There was a problem hiding this comment.
In an earlier comment I asked you to consider some further invalid cases for testing. You've added some nice ones here but I think there's still one thing you haven't covered that is worth checking for. Once you've figured that out, it should be included in your Jest tests also!
There was a problem hiding this comment.
Yes, I have included the invalid check here and added one more number invalid check.
Liam310
added
Reviewed
lintsang
added
the
Needs Review
2 participants
Learners, PR Template
Self checklist
Changelist
I followed the readme.md and finished the 1-implement-and-rewrite-tests folder