London | 26-ITP-Sep | Mandip Sanger | Sprint 3 | Implement-and-rewrire-tests

[mandipsanger]

Self checklist

• I have titled my PR with Region | Cohort | FirstName LastName | Sprint | Assignment Title
• My changes meet the requirements of the task
• I have tested my changes
• My changes follow the style guide

Task code

CYF-1059

Changelist

Sprint 3 coursework

[LonMcGregor]

Almost there, just a couple of small comments to fix

[LonMcGregor]

You marked my comment about the fractions as resolved, but you don't seem to have addressed my comment. Did you understand it?

Also, did you run the tests before comitting and updating the PR? I get errors when I try to run it.

[LonMcGregor]

I am still getting a lot of errors when I run the test. Do you also see these when you run the test command?

[mandipsanger]

I have tested most of them now and will do it onces again. for rank i need to use loop and will push it soon.

[abdishakoor-dev]

The Jest suite runs clean for me now, 19 tests passing, so that part is sorted. getAngleType is solid.

The fraction function is the main thing left.

1. isProperFraction. A fraction is proper when the numerator is smaller than the denominator ignoring the minus signs. So -4/-5 is proper, and so is 1/-5. Your function returns false for both, and your tests on lines 34 and 38 say false is correct. It is the function that needs to change here, and then those two tests. One more to think about: what should isProperFraction(-2, 1) be, and what does your function return for it now?

2. getCardValue accepts some cards it should reject. See my reply on the Number(rank) line.

Add the Needs Review label again when you have pushed.

[abdishakoor-dev]

This guard is what makes -4/-5 and 1/-5 come out false. If you compare the sizes of the two numbers ignoring their signs, do you still need it? Check your two zero-denominator tests still pass after the change.

[abdishakoor-dev]

1/-5 is a proper fraction, because 1 is smaller than 5. This test says false, which matches your code rather than the maths. Line 38 has the same problem.