London | 25-ITP-Sep | Payman Issa Baiglu | sprint 3 | 1- implement-and-rewrite #890
Conversation
PaymanIB
added
the
Needs Review
cjyuan
left a comment
cjyuan
left a comment
There was a problem hiding this comment.
Are these files related to Sprint-3 exercise? If not, can you remove them?
They are making code review less convenient.
If they are local configuration files, you can consider updating .gitignore in the top level folder to exclude them from being tracked by Git. Alternatively, you can selectively manually commit only the relevant files (instead of "commit all changes") to the repository.
| test("should return false for an improper fraction", () =>{ | ||
| expect(isProperFraction(5, 2)).toEqual(false); | ||
| }); | ||
|
|
||
| // Case 3: Identify Negative Fractions: | ||
| test("should return true for negative proper fraction", () => ( | ||
| expect(isProperFraction(-4,7)).toEqual(true) | ||
| )); | ||
|
|
||
| // Case 4: Identify Equal Numerator and Denominator: | ||
| test("should return false when numerator equals denominator", () => { | ||
| expect(isProperFraction(3,3)).toEqual(false); | ||
| }); | ||
|
|
||
| // Stretch: | ||
| // What other scenarios could you test for? | ||
| test("should return true for negative proper fraction with negative denominator", () => { | ||
| expect(isProperFraction(4,-7)).toEqual(true); | ||
| }); |
There was a problem hiding this comment.
What are your expected range of numerators and denominators?
If you test all possible combinations of positive/negative numerator and denominator, you may discover a bug in your implementation.
There was a problem hiding this comment.
Thanks for pointing that out. Yes in some other calculations the output will be undefined. I fixed the function.
There was a problem hiding this comment.
We could make the test more comprehensive by testing all combination of negative and positive parameters.
For example,
test("should return false when numerator and denominator have the same absolute value", () => {
expect(isProperFraction(3,3)).toEqual(false);
expect(isProperFraction(3,-3)).toEqual(false);
expect(isProperFraction(-3,3)).toEqual(false);
expect(isProperFraction(-3,-3)).toEqual(false);
});
There was a problem hiding this comment.
Thanks for clarifying this with an example.
Sprint-3/1-implement-and-rewrite-tests/implement/3-get-card-value.js
Outdated
Show resolved
Hide resolved
| test("should return numeric value for number cards (2-10)", () => { | ||
| const fiveofHearts = getCardValue("5♥"); | ||
| expect(fiveofHearts).toEqual(5); | ||
| }); | ||
| // Case 3: Handle Face Cards (J, Q, K): | ||
| test("should return 10 for face cards (J, Q, K) and 10", () => { | ||
| expect(getCardValue("10♦")).toEqual(10); |
There was a problem hiding this comment.
- 2 and 10 are good boundary cases to test.
- 10 is normally considered a number card.
There was a problem hiding this comment.
CAn you please explain more. So, you mean that I should write a test for 2 as well?
There was a problem hiding this comment.
In your test category for number cards, you only tested one rank, "5".
To make the test more comprehensive, we should test more samples, and boundary cases, 2 and 10, are usually good candidates to test. Since the sample size is small, a test that checks all numbers from 2 to 10 would be even better.
There was a problem hiding this comment.
Thanks for clarifying this. I tried to do both. so first I wrote tests for boundary (2 & 10) which are commented now.. and then wrote a test to cover all numbers.
| test("should return 11 for Ace (A)", () => { | ||
| expect(getCardValue("A♦")).toEqual(11); | ||
| }); |
There was a problem hiding this comment.
This test and the test on lines 7-9 are quite similar.
There was a problem hiding this comment.
Noted, Thank you. I removed this one.
cjyuan
added
Reviewed
|
Thanks @cjyuan , I tried my best to address all the points that you have mentioned. Can you please check it out. |
PaymanIB
added
Needs Review
|
Changes look good. Some of the tests could be made more comprehensive. (You can improve them later when you have time). Can you remove unrelated changes introduced to this branch? |
cjyuan
added
Reviewed
…st for all number cards (2–10)
2 participants
made a new branch so it has no extra files