Skip to content

Cape Town | sep-2026-ITP | Matthaus Bause | Sprint 2 | Implement and rewrite tests - #1685

Open
chefmatthaus-hub wants to merge 1 commit into
CodeYourFuture:mainfrom
chefmatthaus-hub:coursework/sprint-3-clean
Open

chefmatthaus-hub wants to merge 1 commit into
CodeYourFuture:mainfrom
chefmatthaus-hub:coursework/sprint-3-clean

Conversation

@chefmatthaus-hub

Copy link
Copy Markdown

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

@chefmatthaus-hub chefmatthaus-hub added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Oct 12, 2026
@hackertainment hackertainment added Review in progress This review is currently being reviewed. This label will be replaced by "Reviewed" soon. and removed Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. labels Oct 18, 2026

@hackertainment hackertainment left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code generally looks good to me. Just need to fix test cases for proper fraction. Your are almost there, and keep it up :-)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems to me that there are redundant and missing test cases:

  • About zero cases, you have tested (0, 1) (1, 0) and (-1, 0) but missed a case.
  • About non-zero (numerator≠denominator) case, you have included (-1, -2) (1, -2) (-1, 2) and (1, 2) but they are all numerator less than denominator (if not considering the negative sign). How about numerator greater than denominator with negative numbers? Since (9, 8) (4, 2) (5, 3) (6, 4) (7, 1) (15, 14) are all testing the same scenario, there would be 3 other combinations missing.
  • About non-zero (numerator=denominator) case, you have tested positive number (4, 4) but missed the negative number.

So please add the missing cases (and optionally remove redundant cases as well). Thank you.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment as https://github.com/CodeYourFuture/Module-Structuring-and-Testing-Data/pull/1685/changes#r4308748253
Please add the missing cases (and optionally remove redundant cases as well). Thank you.

@hackertainment hackertainment added Reviewed Volunteer to add when completing a review with trainee action still to take. and removed Review in progress This review is currently being reviewed. This label will be replaced by "Reviewed" soon. labels Oct 18, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Reviewed Volunteer to add when completing a review with trainee action still to take.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants