Birmingham | 25-ITP-Sep | Joy Opachavalit | Sprint 3 | coursework/sprint-3#775
Conversation
…t-3-implement-and-rewrite/implement/1-get-angle-type
|
Please hold on creating new PRs for Sprint-3 exercise. I am asking for permission to accept also Sprint-3 exercise submitted in one PR. On separate note, why set the branch name to |
Thanks for the feedback! I initially named the branch that way because I thought a separate PR was required for each file. Later, I realized that wasn’t necessary, but I continued working on the same branch. I wasn’t sure how to rename the branch, so I decided to keep it as is. |
|
We could rename a branch on GitHub. |
| return "Acute angle"; | ||
| } | ||
| // Then keep going for the other cases, one at a time. | ||
| if (angle > 90 && angle < 180) { |
There was a problem hiding this comment.
Checking angle > 90 is optional because previous if-statements (together with the return statements) can guarantee angle is always more than 90.
Sprint-3/1-implement-and-rewrite-tests/implement/2-is-proper-fraction.js
Outdated
Show resolved
Hide resolved
| const numValue = parseInt(rank, 10); | ||
| if (numValue >= 2 && numValue <= 10) { |
There was a problem hiding this comment.
In JavaScript, strings that represent valid numeric literals in the language can be safely converted to equivalent numbers. For examples, "0x02", "2.1", or "0002".
Does your function return the value you expected from each of the following function calls?
getCardValue("0x02♠");
getCardValue("2.1♠");
getCardValue("0002♠");
There was a problem hiding this comment.
With the current function
getCardValue("0x02♠") → undefined
parseInt("0x02", 10) stops at "x" and returns 0, which is outside the 2–10 range, so function returns undefined.
getCardValue("2.1♠") → 2
parseInt("2.1", 10) parses up to the dot and returns 2, which is in the 2–10 range, so function returns 2.
getCardValue("0002♠") → 2
parseInt("0002", 10) returns 2, so the function returns 2.
I changed the code so "0x02" to be interpreted as the JavaScript numeric literal 0x02 (i.e. 2), and reject non-integer ranks like "2.1", by changing the parsing to use Number() and check integerness.### ###
| test("should return false for a negative fraction", () => { | ||
| expect(isProperFraction(-2, 3)).toEqual(false); | ||
| }); |
There was a problem hiding this comment.
In math, -2/3 is considered a proper fraction.
There was a problem hiding this comment.
change the test result to true for-2/3.
| test("should return numeric value for number cards (2-10)", () => { | ||
| const fiveofHearts = getCardValue("5♥"); | ||
| expect(fiveofHearts).toEqual(5); | ||
| }); |
There was a problem hiding this comment.
Could consider testing more sample values in each category (to make the test more comprehensive), especially the boundary cases such as "2♥" and "10♥".
There was a problem hiding this comment.
Added more tests, especially the boundary cases such as "2♥" and "10♥."
| test("should return '2nd' for 2", () => { | ||
| expect(getOrdinalNumber(2)).toEqual("2nd"); | ||
| }); |
There was a problem hiding this comment.
To ensure thorough testing, we need broad scenario coverage. Listing individual values, however, can quickly lead to an unmanageable number of test cases.
Instead of writing tests for individual numbers, consider grouping all possible input values into meaningful categories. Then, select representative samples from each category to test. This approach improves coverage and makes our tests easier to maintain.
For example, we can prepare a test for numbers 2, 22, 132, etc. as
test("append 'nd' to numbers ending in 2, except those ending in 12", () => {
expect( getOrdinalNumber(2) ).toEqual("2nd");
expect( getOrdinalNumber(22) ).toEqual("22nd");
expect( getOrdinalNumber(132) ).toEqual("132nd");
});
There was a problem hiding this comment.
Tests are now grouped together.
Changelist updated. Thank you. |
cjyuan
left a comment
There was a problem hiding this comment.
Changes look good. I have few more suggestions.
Sprint-3/1-implement-and-rewrite-tests/implement/3-get-card-value.js
Outdated
Show resolved
Hide resolved
Sprint-3/1-implement-and-rewrite-tests/rewrite-tests-with-jest/2-is-proper-fraction.test.js
Outdated
Show resolved
Hide resolved
| // All other numbers: should append 'th' | ||
| test("append 'th' to all other numbers", () => { | ||
| expect(getOrdinalNumber(4)).toEqual("4th"); | ||
| expect(getOrdinalNumber(10)).toEqual("10th"); | ||
| expect(getOrdinalNumber(14)).toEqual("14th"); | ||
| expect(getOrdinalNumber(0)).toEqual("0th"); | ||
| }); |
There was a problem hiding this comment.
A more informative description could be "... to numbers ending in 0, 4-9."
There was a problem hiding this comment.
When we run jest test, what message will we see when this test fail?
|
Changes look good. |
Thank you. |
Learners, PR Template
Self checklist
Changelist
-Completed all excercises in Sprint-3