Repository navigation
London | 26-ITP-Sep | Bartosz Kawiak | Sprint 3 | implement-and-rewrite-tests - #1624
bartoszkawiak wants to merge 1 commit into
Conversation
bartoszkawiak
added
Module-Structuring-And-Testing-Data
LonMcGregor
left a comment
LonMcGregor
left a comment
There was a problem hiding this comment.
Good start on this task, The angles task is complete. I have some comments on the others.
| //Unit fractions all have a numerator of 1. | ||
|
|
||
| let validFraction = numerator < denominator && numerator > 0; | ||
| if (validFraction) { |
There was a problem hiding this comment.
This if statement looks a bit complicated. Do you think it could be simplified?
There was a problem hiding this comment.
After looking into it I realised the if statement wasn't needed because the conditions I assigned to validFraction already return either true or false so I can just return the variable directly.
| expect(isProperFraction(-5, 5)).toEqual(false); | ||
| }); | ||
| test(`should return false when ( numerator < negative denominator )`, () => { | ||
| expect(isProperFraction(3, -5)).toEqual(false); |
There was a problem hiding this comment.
In this kind of maths, whenever the numerators value is lower than the denominator, regardless of ± sign, it is considered a true proper fraction. So -4/8 and 3/-5 are both valid proper fractions. Could you take another try at this?
There was a problem hiding this comment.
Thanks for the clarification. I've updated the function to compare the absolute values of the numerator and denominator and added tests to cover the different positive and negative combinations.
| for (let number = 2; number <= 10; number++) { | ||
| for (let suit of suits) { | ||
| test(`should return card value as number`, () => { | ||
| expect(getCardValue(`${number}${suit}`)).toEqual(number); |
There was a problem hiding this comment.
I appreciate the effort gone to test these thoroughly, but one thing to be careful of is if you make your tests so complicated that the tests themselves need testing. As a general rule, it's better to pick out specific cases that test the general input and the edge cases rather than needing to create loops and data structures to test every single possible input.
Do you have nay thoughts on the approach you used here?
There was a problem hiding this comment.
That makes sense. I wanted to check every possible number and suit to make sure they all worked correctly. I figured that using a loop would be the most efficient approach, but I can see how testing a few specific cases would make testing simpler. Thank you for the feedback.
LonMcGregor
added
Reviewed
This comment has been minimized.
This comment has been minimized.
1 similar comment
This comment has been minimized.
This comment has been minimized.
bartoszkawiak
added
the
Needs Review
This comment has been minimized.
This comment has been minimized.
github-actions
Bot
removed
the
Needs Review
bartoszkawiak
added
the
Needs Review
This comment has been minimized.
This comment has been minimized.
github-actions
Bot
removed
the
Needs Review
bartoszkawiak
added
the
Needs Review
This comment has been minimized.
This comment has been minimized.
github-actions
Bot
removed
the
Needs Review
9d8a18b to
aea8177
Compare
bartoszkawiak
added
the
Needs Review
LonMcGregor
left a comment
LonMcGregor
left a comment
There was a problem hiding this comment.
Great,this task is complete now. Good work
LonMcGregor
added
Complete
2 participants
Learners, PR Template
Self checklist
Task code
CYF-1059
Changelist
Completed the implementation exercises first, then added tests using Jest to cover different inputs, outcomes, and edge cases. Fixed issues found during testing and confirmed the tests pass.