Glasgow | 25-ITP-Sep | Hanna Mykytiuk | Sprint 3 | implement-and-rewrite-tests#808
Glasgow | 25-ITP-Sep | Hanna Mykytiuk | Sprint 3 | implement-and-rewrite-tests#808HannaOdud wants to merge 2 commits into
Conversation
HannaOdud
added
📅 Sprint 3
|
Your PR description contained template fields which weren't filled in. Check you've ticked everything in the self checklist, and that any sections which prompt you to fill in an answer are either filled in or removed. If this PR is not coursework, please add the NotCoursework label (and message on Slack in #cyf-curriculum or it will probably not be noticed). |
2 similar comments
|
Your PR description contained template fields which weren't filled in. Check you've ticked everything in the self checklist, and that any sections which prompt you to fill in an answer are either filled in or removed. If this PR is not coursework, please add the NotCoursework label (and message on Slack in #cyf-curriculum or it will probably not be noticed). |
|
Your PR description contained template fields which weren't filled in. Check you've ticked everything in the self checklist, and that any sections which prompt you to fill in an answer are either filled in or removed. If this PR is not coursework, please add the NotCoursework label (and message on Slack in #cyf-curriculum or it will probably not be noticed). |
LonMcGregor
left a comment
LonMcGregor
left a comment
There was a problem hiding this comment.
Good start on this sprint's tasks, I have spotted a few areas where you could improve code further
|
|
||
| function isProperFraction(numerator, denominator) { | ||
| if (numerator < denominator) { | ||
| if (Math.abs(numerator) < Math.abs(denominator)) { |
There was a problem hiding this comment.
Here you are using a common pattern of code:
if (some expression) {
return true
} else {
return false
}This can be simplified into a single line. Can you see how to do that?
There was a problem hiding this comment.
Thanks. I have changet to line: return (Math.abs(numerator) < Math.abs(denominator))
| try { | ||
| assertEquals(getCardValue("W♥"), "Invalid card rank"); | ||
| } catch (error) { | ||
| console.log(error.message); |
There was a problem hiding this comment.
Looking at the jest documentation, can you see any expect() options that would let you test for exceptions?
There was a problem hiding this comment.
Thanks, I have changed to expect(() => { getCardValue("21♠") }).toThrow("Invalid card rank.");
LonMcGregor
added
Reviewed
|
Your PR description contained template fields which weren't filled in. Check you've ticked everything in the self checklist, and that any sections which prompt you to fill in an answer are either filled in or removed. If this PR is not coursework, please add the NotCoursework label (and message on Slack in #cyf-curriculum or it will probably not be noticed). |
1 similar comment
|
Your PR description contained template fields which weren't filled in. Check you've ticked everything in the self checklist, and that any sections which prompt you to fill in an answer are either filled in or removed. If this PR is not coursework, please add the NotCoursework label (and message on Slack in #cyf-curriculum or it will probably not be noticed). |
HannaOdud
added
Needs Review
|
Your PR description contained template fields which weren't filled in. Check you've ticked everything in the self checklist, and that any sections which prompt you to fill in an answer are either filled in or removed. If this PR is not coursework, please add the NotCoursework label (and message on Slack in #cyf-curriculum or it will probably not be noticed). |
1 similar comment
|
Your PR description contained template fields which weren't filled in. Check you've ticked everything in the self checklist, and that any sections which prompt you to fill in an answer are either filled in or removed. If this PR is not coursework, please add the NotCoursework label (and message on Slack in #cyf-curriculum or it will probably not be noticed). |
LonMcGregor
left a comment
LonMcGregor
left a comment
There was a problem hiding this comment.
Great work on these changes. This is complete now
LonMcGregor
added
Complete
|
Your PR description contained template fields which weren't filled in. Check you've ticked everything in the self checklist, and that any sections which prompt you to fill in an answer are either filled in or removed. If this PR is not coursework, please add the NotCoursework label (and message on Slack in #cyf-curriculum or it will probably not be noticed). |
1 similar comment
|
Your PR description contained template fields which weren't filled in. Check you've ticked everything in the self checklist, and that any sections which prompt you to fill in an answer are either filled in or removed. If this PR is not coursework, please add the NotCoursework label (and message on Slack in #cyf-curriculum or it will probably not be noticed). |
3 participants
Learners, PR Template
Self checklist
Changelist
Briefly explain your PR.
Practicing tests ang Jest test.
Questions
Ask any questions you have for your reviewer.