London | 25-ITP-Sep | Gislaine Della Bella | Sprint 3 | Implement-and-rewrite-tests #788
Conversation
|
Your PR's title didn't contain a known region. Please check the expected title format, and make sure your region is in the correct place and spelled correctly. 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). |
Della-Bella
added
the
Needs Review
|
Your PR's title didn't contain a known region. Please check the expected title format, and make sure your region is in the correct place and spelled correctly. 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). |
cjyuan
left a comment
cjyuan
left a comment
There was a problem hiding this comment.
Can you update the title of this PR to make it match the format described in the PR template?
Also, there were four checkboxes and a "Changelist" section in the PR template. Can you include them in your PR description? You can copy the Markdown code in the PR template from
https://github.com/Della-Bella/Module-Structuring-and-Testing-Data/blob/coursework/sprint-3-implement-and-rewrite/.github/pull_request_template.md
| } else if (angle > 90 && angle < 180) { | ||
| return "Obtuse angle"; |
There was a problem hiding this comment.
Both the else keyword and checking angle > 90 are optional.
Can you figure out why?
There was a problem hiding this comment.
hello Cyjuan,
Yes now i can see the redundante. I refractored the code and took the reduntant check on the 90 angule. thank you
| if (numerator < denominator) { | ||
| return true; | ||
| return true ; | ||
| } | ||
| else { | ||
| return false; |
There was a problem hiding this comment.
According to the definition of proper fraction in mathematics:
isProperFraction(-4, 3)should returnfalseisProperFraction(-2, 5)should returntrueisProperFraction(-1, 1)should returnfalseisProperFraction(-2, -3)should returntrue
Can you look up the definition of proper fraction and update your function accordingly?
| } else if (rank >= "2" && rank <= "9") { | ||
| return parseInt(rank); |
There was a problem hiding this comment.
Does your function return the value or behave the way you expected from each of the following function calls?
getCardValue("20♠");
getCardValue("2.1♠");
getCardValue("0002♠");
| test("returns Obtuse angle for values greater than 90 degrees and less than 180 degrees",()=>{ | ||
| expect(getAngleType(120)).toEqual("Obtuse angle"); | ||
| }); | ||
|
|
||
|
|
||
| // Case 4: Identify Straight Angles: | ||
| // When the angle is exactly 180 degrees, | ||
| // Then the function should return "Straight angle" | ||
|
|
||
| test("returns Straight angle for angle === 180",() =>{ | ||
| expect(getAngleType(180)).toEqual("Straight angle"); | ||
| }); | ||
|
|
||
| // Case 5: Identify Reflex Angles: | ||
| // When the angle is greater than 180 degrees and less than 360 degrees, | ||
| // Then the function should return "Reflex angle" | ||
|
|
||
| test("returns Reflex angle for angles > 180 && angle < 360",() =>{ | ||
| expect(getAngleType(290)).toEqual("Reflex angle"); | ||
| }); |
There was a problem hiding this comment.
Indentation is not very consistent.
|
|
||
| // Case 2: Identify Improper Fractions: | ||
|
|
||
| test("it is a Improper Fraction",() =>{ |
There was a problem hiding this comment.
The test description is not quite informative; it does not describe what the function should return or behave when the input represents an improper fraction.
Ideally, the description should help a developer figure out how to implement a function to pass all the tests.
| }); | ||
|
|
||
| // Case 3: Handle Face Cards (J, Q, K): | ||
| test ("should return Face Cards J, Q, K ", () =>{ |
There was a problem hiding this comment.
What should the function return when the input represents a face card?
| test("should return number cards", () => { | ||
| const fiveofHearts = getCardValue("5♥"); | ||
| expect(fiveofHearts).toEqual(5); | ||
| }); |
There was a problem hiding this comment.
Here's an example of how we can specify a test for number cards.
test("should return the value of number cards (2-10)", () => {
expect(getCardValue("2♣︎")).toEqual(2);
expect(getCardValue("5♠")).toEqual(5);
expect(getCardValue("10♥")).toEqual(10);
// Note: We could also use a loop to check all values from 2 to 10.
});
Note: We could test multiple samples within each test to make the test more comprehensive.
cjyuan
added
Reviewed
|
Your PR's title didn't contain a known region. Please check the expected title format, and make sure your region is in the correct place and spelled correctly. 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's title didn't contain a known region. Please check the expected title format, and make sure your region is in the correct place and spelled correctly. 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). |
Della-Bella
changed the title
|
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 participants
Learners, PR Template
Self checklist
Changelist
Briefly explain your PR.
Questions
Ask any questions you have for your reviewer.