Skip to content

London | 26 ITP SEP | Russom Gebremeskel | Sprint 2 | Implement and Rewrite Tests - #1700

Open
russom-g wants to merge 68 commits into
CodeYourFuture:mainfrom
russom-g:coursework/sprint-2-implement-and-rewrite
Open

russom-g wants to merge 68 commits into
CodeYourFuture:mainfrom
russom-g:coursework/sprint-2-implement-and-rewrite

Conversation

@russom-g

@russom-g russom-g commented Oct 13, 2026 •

Copy link
Copy Markdown

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

Task code

CYF-1059

Changelist

Functions implemented. Tests written with node and jest.

@github-actions

This comment has been minimized.

@russom-g russom-g added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Oct 13, 2026
@github-actions

This comment has been minimized.

@github-actions github-actions Bot removed the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Oct 13, 2026
@github-actions

This comment has been minimized.

1 similar comment
@github-actions

This comment has been minimized.

@russom-g russom-g added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Oct 13, 2026
@russom-g
russom-g force-pushed the coursework/sprint-2-implement-and-rewrite branch from 7d498f5 to 0c75ac6 Compare October 13, 2026 22:08
@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 19, 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.

Thank you for your coursework. Just an additional note: a decimal number itself is a fraction, but decimal fraction may not be well defined without simplified into integer fraction in mathematics. Therefore, I will let you pass this time as this is a programming exercise but not a mathematics course (and you did return the results correctly if I calculate their integer form on my own).

return "Obtuse angle";
} else if (angle === 180) {
return "Straight angle";
} else if (angle > 180 && angle <= 360) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

May possibly be just a typo, but read line 10 of the question carefully and please fix your code as well. Thank you.

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.

Thanks for the review and feedback.

= sign is removed from reflex angle if condition statement.

Comment on lines +48 to +49
const Reflex2 = getAngleType(360);
assert.equal(Reflex2, "Reflex angle");

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This looks not quite correct to me, and please fix it as well. Thank you.

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.

Its changed to 359.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Hmm... you would also need to change the boundary case of invalid angle as well.

test(`should return "Reflex angle" when (180 > angle <= 360)`, () => {
expect(getAngleType(181)).toEqual("Reflex angle");
expect(getAngleType(299)).toEqual("Reflex angle");
expect(getAngleType(360)).toEqual("Reflex angle");

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This line looks not quite correct to me, and please fix it as well. Thank you.

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.

It'a changed to 359.

@hackertainment hackertainment Oct 20, 2026 •

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Hmm... you would also need to change the test description and the boundary case of invalid angle as well.

Comment on lines +23 to +25
test("Negative numerator proper fraction", () => {
assert.equal(isProperFraction(-9, 1), true);
});

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

this one should return false, please fix it, thank you.

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.

Done.

Comment on lines +27 to +29
test("Both negative proper fraction", () => {
assert.equal(isProperFraction(-7, -3), true);
});

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

this one should return false, please fix it, thank you.

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.

Done.

@hackertainment hackertainment Oct 20, 2026 •

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Thank you. For both negative numerator and denominator, you have included (-4, -4) and (-7, -3), but how about (-3, -7) where the "result" is a positive number?

Comment on lines +26 to +28
test(`should return true for negative proper fraction`, () => {
expect(isProperFraction(-7, -3)).toEqual(true);
});

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

this one should return false, please fix it, thank you.

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.

Done.

@hackertainment hackertainment Oct 20, 2026 •

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Thank you. For both negative numerator and denominator, you have included (-4, -4) and (-7, -3), but how about (-3, -7) where the "result" is a positive number?

Comment on lines +22 to +24
test(`should return true for negative numerator proper fraction`, () => {
expect(isProperFraction(-9, 1)).toEqual(true);
});

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

this one should return false, please fix it, thank you.

@russom-g russom-g Oct 19, 2026 •

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.

Done.

The reason I included negative fraction is the requirement didn't say no negative numbers. Secondly I checked online and I read that negative numbers can be proper fraction.

@hackertainment hackertainment Oct 20, 2026 •

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Yes, negative numbers can be proper fraction if (without the negative sign) the numerator is smaller than the denominator. As a result, both (-1, 9) and (1, -9) should return true but (-9, 1) and (9, -1) should return false. The issue here is inconsistency. Either consistently treating end result between -1 and 0 as true if negative numbers can be proper fraction, or consistently treating all negative "result" as false if negative numbers cannot be proper fraction.

p.s.: for this question, you have to handle negative numbers even the question didn't mentioned it.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I don't think this program is completely correct. Consider card = "AA", would the result be the same as expected?

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 added suits and validSuits variables to fix the bug.

Many thanks

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Much better now.

@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 19, 2026
@russom-g russom-g added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Oct 19, 2026
@hackertainment hackertainment removed the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Oct 20, 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