Skip to content

Conversation

@CatchingKiyoko
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

Changelist

  1. refactored and added lines of code
  2. answered questions in comments

…CardValue function with comments explaining function and updated tests
@CatchingKiyoko CatchingKiyoko 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 Nov 13, 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.

  • Code is pretty solid.

  • You missed implementing the Jest tests in Sprint-3/1-implement-and-rewrite-tests/rewrite-tests-with-jest.

Comment on lines 10 to 13
function isProperFraction(numerator, denominator) {
if (numerator < denominator) {
return true;
}
if (numerator < denominator) return true;
else return false;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

According to the definition of proper fraction in mathematics:

  • isProperFraction(-4, 3) should return false
  • isProperFraction(-2, 5) should return true
  • isProperFraction(-2, 0) should return false
  • isProperFraction(-1, 1) should return false
  • isProperFraction(-2, -3) should return true

Can you look up the definition of proper fraction and update your function accordingly?

if (rank === "A") {
return 11;
}
var rank = card.slice(0, -1); // get the rank of the card by removing the last character. (the suit is the last character)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not use let to declare the variable?

Comment on lines 12 to 27
if (rank === "A") return 11; // this checks for Aces
// Handle Number Cards (2-9)
if (rank === "2") return 2; // this checks for the twos
if (rank === "3") return 3; // this checks for the threes
if (rank === "4") return 4; // this checks for the fours
if (rank === "5") return 5; // this should check for fives
if (rank === "6") return 6; // this checks for the sixes
if (rank === "7") return 7; // this checks for the sevens
if (rank === "8") return 8; // this checks for the eights
if (rank === "9") return 9; // this checks for the nines
// Handle Face Cards (J, Q, K) And 10's
if (rank === "J") return 10; // this checks for Jacks
if (rank === "Q") return 10; // this checks for Queens
if (rank === "K") return 10; // this checks for Kings
if (rank === "10") return 10; // this checks for Tens
// if none of the above its an invalid card and throw an error
Copy link
Contributor

Choose a reason for hiding this comment

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

This is very solid. As a stretch exercise, can you figure out a way to simplify these code (e.g., reduce the number of if statements involved)?

Copy link
Author

Choose a reason for hiding this comment

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

by mapping each of the card ranks directly to values so instead of using if statements and using an object to make it more scalable and cleaner.

Copy link
Contributor

@cjyuan cjyuan Nov 15, 2025

Choose a reason for hiding this comment

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

by mapping each of the card ranks directly to values so instead of using if statements and using an object to make it more scalable and cleaner.

How would you express them in JS? And what do you mean by "scalable"?

Copy link
Author

Choose a reason for hiding this comment

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

by making a lookup object by making a const of the card values and then using the key to pull each of the ranks and this would make it have no if-statements. and its more scalable in the sence of if needing to add cards

Comment on lines 14 to 28
// Case 2: Identify the ordinal number for 2
// When the number is 2,
// The function should then return "2nd".

test("Should return `2nd` for 2", () => {
expect(getOrdinalNumber(2)).toEqual("2nd");
});

// Case 3: Identify the ordinal number for 3
// When the number is 3,
// The Function should the return "3rd"

test("Should return `3rd` for 3", () => {
expect(getOrdinalNumber(3)).toEqual("3rd");
});
Copy link
Contributor

Choose a reason for hiding this comment

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

To ensure thorough testing, we need broad scenarios that cover all possible cases.
Listing individual values, however, can quickly lead to an unmanageable number of test cases.
Instead of writing tests for individual numbers, consider grouping all possible input values into meaningful categories.
Then, select representative samples from each category to test. This approach improves coverage and makes our tests easier to maintain.

For example, we can prepare a test for numbers 2, 22, 132, etc. as

test("append 'nd' to numbers ending in 2, except those ending in 12", () => {
    expect( getOrdinalNumber(2) ).toEqual("2nd");
    expect( getOrdinalNumber(22) ).toEqual("22nd");
    expect( getOrdinalNumber(132) ).toEqual("132nd");
});

Can you update the tests in this script to make them more comprehensive? That is, create few test categories that can cover all possible cases?

Copy link
Author

Choose a reason for hiding this comment

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

have updated them to have more tests to cover more possible cases

@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 Nov 15, 2025
@CatchingKiyoko CatchingKiyoko 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 Nov 15, 2025
Comment on lines +11 to +12
if (denominator === 0) return false; // avoid division by zero
return Math.abs(numerator) < Math.abs(denominator);
Copy link
Contributor

Choose a reason for hiding this comment

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

There is no "division by zero" needs to be prevented though.

Will the function still work equally well without this if statement?

Copy link
Author

Choose a reason for hiding this comment

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

yes it would still work because it would return false implicitly with out the if-statement, as im currently explicitly looking for the error having the if-statement in.

Comment on lines 12 to 27
if (rank === "A") return 11; // this checks for Aces
// Handle Number Cards (2-9)
if (rank === "2") return 2; // this checks for the twos
if (rank === "3") return 3; // this checks for the threes
if (rank === "4") return 4; // this checks for the fours
if (rank === "5") return 5; // this should check for fives
if (rank === "6") return 6; // this checks for the sixes
if (rank === "7") return 7; // this checks for the sevens
if (rank === "8") return 8; // this checks for the eights
if (rank === "9") return 9; // this checks for the nines
// Handle Face Cards (J, Q, K) And 10's
if (rank === "J") return 10; // this checks for Jacks
if (rank === "Q") return 10; // this checks for Queens
if (rank === "K") return 10; // this checks for Kings
if (rank === "10") return 10; // this checks for Tens
// if none of the above its an invalid card and throw an error
Copy link
Contributor

@cjyuan cjyuan Nov 15, 2025

Choose a reason for hiding this comment

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

by mapping each of the card ranks directly to values so instead of using if statements and using an object to make it more scalable and cleaner.

How would you express them in JS? And what do you mean by "scalable"?

Comment on lines +15 to +17
test("should return true for a negative proper fraction", () => {
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.

Could consider introduce a test for negative improper fraction to make the test more complete.

Copy link
Author

Choose a reason for hiding this comment

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

have implemented the test for negative improper fractions.

@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 Nov 15, 2025
Added test cases for negative improper fractions.
@CatchingKiyoko CatchingKiyoko 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 Nov 19, 2025
@cjyuan
Copy link
Contributor

cjyuan commented Nov 19, 2025

Looks 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. labels Nov 19, 2025
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. Module-Structuring-And-Testing-Data The name of the module. 📅 Sprint 3 Assigned during Sprint 3 of this module

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants