Skip to content

Sheffield | 26-Jan-ITP | Seti Mussa | sprint 3 | coursework implement and rewrite #1278

Open
Seti-Jemal wants to merge 12 commits intoCodeYourFuture:mainfrom
Seti-Jemal:coursework-sprint-3--implement-and-rewrite
Open

Sheffield | 26-Jan-ITP | Seti Mussa | sprint 3 | coursework implement and rewrite #1278
Seti-Jemal wants to merge 12 commits intoCodeYourFuture:mainfrom
Seti-Jemal:coursework-sprint-3--implement-and-rewrite

Conversation

@Seti-Jemal
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

Changelist

I have completed the Sprint 3 implement and rewrite test

Questions

N/A

@Seti-Jemal Seti-Jemal added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Mar 21, 2026
@Seti-Jemal Seti-Jemal closed this Mar 21, 2026
@Seti-Jemal Seti-Jemal reopened this Mar 21, 2026
@Seti-Jemal Seti-Jemal changed the title Sheffield | 26-Jan-ITP | Seti Mussa | sprint 3 | coursework implement and rewrite Sheffield | 26-Jan-ITP | Seti Mussa | sprint 3 | coursework implement and rewrite Mar 21, 2026
@Seti-Jemal Seti-Jemal added 📅 Sprint 3 Assigned during Sprint 3 of this module Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. and removed 📅 Sprint 3 Assigned during Sprint 3 of this module Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. labels Mar 21, 2026
Copy link
Copy Markdown
Contributor

@cjyuan cjyuan left a comment

Choose a reason for hiding this comment

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

  • The isProperFraction() function is not quite correct. The other two functions are good.

  • This exercise also expects you to implement the Jest tests in the rewrite-tests-with-jest folder.

Comment on lines +34 to +44
assertEquals(isProperFraction(3, 5), true);
assertEquals(isProperFraction(4, 8), true);
assertEquals(isProperFraction(-1, 2), true);
assertEquals(isProperFraction(-1, -2), true);
assertEquals(isProperFraction(1, -2), true);

//Not
assertEquals(isProperFraction(9, 7), false);
assertEquals(isProperFraction(13, 11), false);
assertEquals(isProperFraction(19, 10), false);
assertEquals(isProperFraction(17, 3), false);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

  • Your current function cannot yet pass all these tests.

  • You can also test some improper fractions with negative numerator or/and denominator.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I have applied the feedbacks.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

  • If you have made any new commit, don't forget to push the commit also to GitHub.

  • If your PR is ready to be re-reviewed, don't forget to add the "Needs Review" label.

@cjyuan cjyuan added Reviewed Volunteer to add when completing a review with trainee action still to take. and removed Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. labels Mar 21, 2026
@Seti-Jemal Seti-Jemal added Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. and removed Reviewed Volunteer to add when completing a review with trainee action still to take. labels Mar 27, 2026
Copy link
Copy Markdown
Contributor

@cjyuan cjyuan left a comment

Choose a reason for hiding this comment

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

Changes look good.

Comment on lines +18 to +35
test(`should return true when numerator is less than denominator`, () => {
expect(isProperFraction(5, 9)).toEqual(true);
});

// Special case; negative numerator and positive denominatore
test(` should return true when numertaor is negative and less than demoniator`, () => {
expect(isProperFraction(-5, 9)).toEqual(true);
});

// special case: postive numerator and negative deminator
test(`should return true when numerator is positive and less than denominator`, () => {
expect(isProperFraction(5, -9)).toEqual(true);
});

// Special case: negative numerator and negative denominator
test(`should return true when both are negative`, () => {
expect(isProperFraction(-5, -9)).toEqual(true);
});
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

These descriptions are not quite correct without mentioning the comparison is to be made on their absolute value. For example, 5 is actually more than -9, but abs(5) < abs(9).

Note: We can use pseudo-code and notations like abs(...) or | ... | in the descriptions to more
concisely describe the conditions (the "when" part).

@cjyuan cjyuan added Complete Volunteer to add when work is complete and all review comments have been addressed. and removed Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. labels Mar 27, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Complete Volunteer to add when work is complete and all review comments have been addressed.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants