Skip to content

Glasgow | ITP May -25 | Mirabelle Morah | Module-Structuring-and-Testing-Data | Coursework/sprint-3 #628

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

mirabellemorah
Copy link

Learners, PR Template

Self checklist

  • I have committed my files one by one, on purpose, and for a reason
  • I have titled my PR with REGION | COHORT_NAME | FIRST_NAME LAST_NAME | PROJ_NAME
  • I have tested my changes
  • My changes follow the style guide
  • My changes meet the requirements of this task

Changelist

Provided answers to sprint 3

Questions

Nil

@mirabellemorah mirabellemorah added 📅 Sprint 3 Assigned during Sprint 3 of this module Needs Review Participant to add when requesting review labels Jun 30, 2025
Comment on lines +3 to +4
if (numerator < denominator) return true;
if (numerator >= denominator) 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.

In mathematics, -A/B == A/-B == -(A/B), and -A/-B == A/B for any integers A and B (B ≠ 0).
They represent a proper fraction if A < B and A ≠ 0 and B ≠ 0.

So isProperFraction(-4, 3) should return false because 4 >= 3.

Consider comparing the absolute value of the numerator and the denominator instead.


We can also use else at line 13 to handle any number that does not satisfy the condition of the if statement at line 12.

Comment on lines +6 to +8
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. For examples, "0x02", "2.1", or "00_02".

Does your function return the value you expected from each of the following function calls?

getCardValue("0x02♠");
getCardValue("2.1♠");
getCardValue("00_02♠");

Comment on lines +9 to +13
test("should return 5 for 5 of Hearts", () => {
const fiveofHearts = getCardValue("5♥");
expect(fiveofHearts).toEqual(5);
});

Copy link
Contributor

Choose a reason for hiding this comment

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

When preparing tests, we should ensure the tests cover all possible cases. If we specify a test for individual card, we will need about 53 tests to cover all possible cases. Instead, we could consider classifying all possible values into different categories, and then within each category we test some samples.

For example, one possible category for getCardValue() is, "should return the value of number cards (2-10)", and we can prepare the test as

test("should return the value of number cards (2-10)", () => {
    expect(getCardValue("2♣︎")).toEqual(2);
    expect(getCardValue("5♠")).toEqual(5);
    expect(getCardValue("10♥")).toEqual(5);
    // Note: We could also use a loop to check all values from 2 to 10.
});

Comment on lines +15 to +19
test("should return 10 for Jack of Diamonds", () => {
const jackofDiamonds = getCardValue("J♦");
expect(jackofDiamonds).toEqual(10);
});

Copy link
Contributor

Choose a reason for hiding this comment

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

We could generalise this test to "should return 10 for face cards (J, Q, K)" and check all three ranks J, Q, K).


return (
num +
(suffixes[(remainder - 20) % 10] || suffixes[remainder] || suffixes[0])
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explain how this expression work?

Comment on lines +15 to +17
test("should return '2nd' for 2", () => {
expect(getOrdinalNumber(2)).toEqual("2nd");
});
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 scenario coverage. 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 so that they cover all possible valid integers?

@cjyuan cjyuan added Reviewed Volunteer to add when completing a review and removed Needs Review Participant to add when requesting review labels Jul 8, 2025
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 📅 Sprint 3 Assigned during Sprint 3 of this module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants