London | 26-ITP-Sep | Bartosz Kawiak | Sprint 3 | implement-and-rewrite-tests - #1624
bartoszkawiak wants to merge 6 commits into
Conversation
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?
| 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?
| 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?
|
The files changed in this PR don't match what is expected for this task. Please check that you committed the right files for the task, and that there are no accidentally committed files from other sprints. Please review the 'files changed' tab at the top of the page. Here is an example of a file that has been changed on this branch but shouldn't be: 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). If this PR needs reviewed, please add the 'Needs Review' label to this PR after you have resolved the issues listed above. |
1 similar comment
|
The files changed in this PR don't match what is expected for this task. Please check that you committed the right files for the task, and that there are no accidentally committed files from other sprints. Please review the 'files changed' tab at the top of the page. Here is an example of a file that has been changed on this branch but shouldn't be: 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). If this PR needs reviewed, please add the 'Needs Review' label to this PR after you have resolved the issues listed above. |
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.