Skip to content

Conversation

@shaghayeghfar
Copy link

Learners, PR Template

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

@shaghayeghfar shaghayeghfar added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Oct 28, 2025
Copy link
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.

  • Why not include a brief description of the changes made in this PR in the "Changelist" section of this PR description?

Comment on lines 26 to 29
const num = Number(rank);
if (num >= 2 && num <= 9) {
return num;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

In JavaScript, strings that represent valid numeric literals in the language can be safely
converted to equivalent numbers or parsed into a valid integers.
Do you want to recognize these string values as valid ranks?

To find out what these strings are, you can ask AI

What kinds of string values would make Number(rank) evaluate to 2 in JS?

Copy link
Author

Choose a reason for hiding this comment

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

I did the changes in the code since we just need specific number and letters in our exercise.
thank you for your help

Comment on lines +29 to +32
// Case 5: Negative denominator
test("should return true when denominator is negative and proper", () => {
expect(isProperFraction(2, -5)).toEqual(true);
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Can your function pass this test?

Copy link
Author

Choose a reason for hiding this comment

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

I change my code
from if (Math.abs(numerator) < (denominator))
to if (Math.abs(numerator) < Math.abs(denominator))

so that it also works with negative denominators.

Comment on lines +15 to +17
test("should return true for a negative proper fraction (numerator < denominator)", () => {
expect(isProperFraction(-4, 7)).toEqual(true);
});
Copy link
Contributor

Choose a reason for hiding this comment

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

What about negative improper fraction?

Copy link
Author

Choose a reason for hiding this comment

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

I made some changes to the is-proper-fraction function , it works correctly now.
Thank you for your guides

@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 Oct 28, 2025
@shaghayeghfar
Copy link
Author

@cjyuan

Dear Cj
Thank you for reviewing my code and for your helpful feedback.
I’ve made the changes you suggested and tests work properly now. thank you again!
I really appreciate your time and support .

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