Skip to content

Manchester | 26-ITP-JAN | Abdu Mussa | Sprint 3 |1-implement-and-rewriting-tests/#1141

Open
Abduhasen wants to merge 6 commits intoCodeYourFuture:mainfrom
Abduhasen:coursework/sprint-3-implement-and-rewrite
Open

Manchester | 26-ITP-JAN | Abdu Mussa | Sprint 3 |1-implement-and-rewriting-tests/#1141
Abduhasen wants to merge 6 commits intoCodeYourFuture:mainfrom
Abduhasen:coursework/sprint-3-implement-and-rewrite

Conversation

@Abduhasen
Copy link

@Abduhasen Abduhasen commented Mar 3, 2026

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

Changelist

implement and rewriting of tests

Questions

N/A

@Abduhasen Abduhasen 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. Module-Structuring-And-Testing-Data The name of the module. labels Mar 3, 2026
@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 11, 2026
@Abduhasen Abduhasen added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Mar 11, 2026
cjyuan
cjyuan reviewed Mar 12, 2026
View reviewed changes
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.

Changes look good. Well done.

@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. Reviewed Volunteer to add when completing a review with trainee action still to take. labels Mar 12, 2026
@Abduhasen
adding abs(...) notation to indicate the comparison is made on the…
ec69eea 
… absolute values of the numerator and the denominator.
ykamal
ykamal reviewed Mar 12, 2026
View reviewed changes
Sprint-3/1-implement-and-rewrite-tests/implement/1-get-angle-type.js

function getAngleType(angle) {
// TODO: Implement this function
if (angle > 0 && angle < 90) return "Acute angle";
Copy link

@ykamal ykamal Mar 12, 2026

Choose a reason for hiding this comment

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

There are quite a lot of conditionals in here. Do you think there is something other than if-else that could make this simpler, cleaner?

ykamal
ykamal reviewed Mar 12, 2026
View reviewed changes
Sprint-3/1-implement-and-rewrite-tests/implement/1-get-angle-type.js
// Example: Identify Right Angles
const right = getAngleType(90);
assertEquals(right, "Right angle");
let acute = getAngleType(1);
Copy link

@ykamal ykamal Mar 12, 2026

Choose a reason for hiding this comment

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

This is very readable, and there is nothing wrong with that. But do you think you need to declare/reassign a variable for each test? How could you make this simpler and less lines of code?

ykamal
ykamal reviewed Mar 12, 2026
View reviewed changes
Sprint-3/1-implement-and-rewrite-tests/implement/2-is-proper-fraction.js Outdated

function isProperFraction(numerator, denominator) {
// TODO: Implement this function
if (Math.abs(numerator) < Math.abs(denominator)) return true;
Copy link

@ykamal ykamal Mar 12, 2026

Choose a reason for hiding this comment

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

I think this could be done in one statement. How do you think it could be done? it'll make the code much simpler as well.

Copy link

@ykamal ykamal Mar 12, 2026

Choose a reason for hiding this comment

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

Also, since these are meant to be used in a division, do you think there needs to be a check to see if the potential division could cause any errors? According to Maths, one of these numbers MUST not be a certain number, or else the division is undefined or erroneous.
(If I'm being vague that's because otherwise it'll reveal the answer 😳 )

ykamal
ykamal reviewed Mar 12, 2026
View reviewed changes
Sprint-3/1-implement-and-rewrite-tests/implement/3-get-card-value.js
throw new Error("Invalid card"); // This if statement checks the suit is valid with the Array of suit we assigned
}
// Based on the question i get form mentor i changed the function if statement for better experience of the code.
const cardValues = {
Copy link

@ykamal ykamal Mar 12, 2026

Choose a reason for hiding this comment

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

Do you think there is a better data type to use for a set of cards? 😉

@cjyuan
Copy link
Contributor

cjyuan commented Mar 12, 2026

@ykamal This PR has already been reviewed. Is there any reason you are reviewing it again?

cjyuan
cjyuan reviewed Mar 12, 2026
View reviewed changes
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.

I just noticed there are some mistakes in the test descriptions. Can you fixed them?

@Abduhasen Abduhasen added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Mar 12, 2026
cjyuan
cjyuan reviewed Mar 13, 2026
View reviewed changes
Sprint-3/1-implement-and-rewrite-tests/rewrite-tests-with-jest/2-is-proper-fraction.test.js Outdated
Comment on lines 13 to 17
test("should return false when abs(numerator) == 0", () => {
expect(isProperFraction(0, -1)).toEqual(true);
expect(isProperFraction(0, 3)).toEqual(true);
expect(isProperFraction(0, -2)).toEqual(true);
});
Copy link
Contributor

@cjyuan cjyuan Mar 13, 2026

Choose a reason for hiding this comment

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

Can you fix this description? It was correct before.

@cjyuan cjyuan added Reviewed Volunteer to add when completing a review with trainee action still to take. and removed Complete Volunteer to add when work is complete and all review comments have been addressed. Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. labels Mar 13, 2026
@Abduhasen Abduhasen added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Mar 13, 2026
@cjyuan
Copy link
Contributor

cjyuan commented Mar 13, 2026

All good now.

@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. Reviewed Volunteer to add when completing a review with trainee action still to take. labels Mar 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cjyuan cjyuan cjyuan left review comments

+1 more reviewer

@ykamal ykamal ykamal requested changes

Reviewers whose approvals may not affect merge requirements

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

Complete Volunteer to add when work is complete and all review comments have been addressed. Module-Structuring-And-Testing-Data The name of the module. 📅 Sprint 3 Assigned during Sprint 3 of this module

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Abduhasen @cjyuan @ykamal