Skip to content

London |26-ITP-Sep| Maharit Belai| Sprint 2 | Implement and rewrite tests - #1716

Open
MaharitB wants to merge 34 commits into
CodeYourFuture:mainfrom
MaharitB:implement-and-rewrite-tests
Open

MaharitB wants to merge 34 commits into
CodeYourFuture:mainfrom
MaharitB:implement-and-rewrite-tests

Conversation

@MaharitB

@MaharitB MaharitB commented Oct 15, 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

I have implemented and wrote tests.

Questions

…the input can be posetive before they are compared
@github-actions

This comment has been minimized.

@MaharitB MaharitB added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Oct 15, 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 15, 2026
@MaharitB MaharitB changed the title London | 26-ITP-Sep| Maharit Belai | Sprint 2 | Implement-and-rewrite-tests London |26-ITP-Sep| Maharit Belai| Sprint 2 | Implement-and-rewrite-tests Oct 15, 2026
@github-actions

This comment has been minimized.

@MaharitB MaharitB changed the title London |26-ITP-Sep| Maharit Belai| Sprint 2 | Implement-and-rewrite-tests London |26-ITP-Sep| Maharit Belai| Sprint 2 | Implement and rewrite tests Oct 15, 2026
@github-actions

This comment has been minimized.

@MaharitB MaharitB added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Oct 15, 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 15, 2026
@MaharitB MaharitB added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Oct 15, 2026
@hey-hammad hey-hammad 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 20, 2026

@hey-hammad hey-hammad 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.

Good effort overall, a few issues requires attention, regarding code quality and test cases. Thanks


export function isProperFraction(numerator, denominator) {
// TODO: Implement this function
if (Math.abs(numerator) < Math.abs(denominator) && numerator !== 0) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

If 0,5 is passed to the fn, what is will be the output of the function? is it the correct output?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

also please add the above input as one of the test cases.

@@ -24,5 +24,52 @@
// execute the code to ensure all tests pass.

export function getCardValue(card) {

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 overall code is good and works fine, however few things can be improved to make it better.

  • do we need to use let for defining the suit and cardInput? do we expect the value of these variables to be changed later? If not is there another way of defining the variables that's better suited for this scenario?

"K",
];

if (suit == "♠" || suit == "♥" || suit == "♣" || suit == "♦") {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Since you already have all the validSuit defined in an array, would it make more sense to check the if the suit already exist in the array rather than checking individually?

return 10;
}
}
if (card == "invalid") {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

is it better to perform all the validation before or after any calculations? which is better?

// TODO: Write tests in Jest syntax to cover all possible outcomes.

// Case 1: Ace (A)
test(`Should return 11 when given an ace card`, () => {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Since we've added a ton of validation for checking if the input is correct, wouldn't it make more sense to test it to confirm if its working fine?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Think about malformed inputs etc.

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.

Thank you for the constructive feedback . I really appreciate it.
I included tests that prove if the validation works.It is at the bottom the of test file.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@MaharitB couple of test including malformed inputs like having empty spaces at the start or end of string will make the test suits more robust. Thanks

@hey-hammad hey-hammad 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 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