Skip to content

Conversation

@kohanman
Copy link

@kohanman kohanman commented Nov 8, 2025

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

Completed the implement and rewrite test coursework

Questions

N/A

@kohanman kohanman added 🐇 Size Small Around an hour 📅 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 8, 2025
Comment on lines 10 to 21
function getAngleType(angle) {
if (angle === 90) {
return "Right angle";
} else if (angle === 45) {
return "Acute angle";
} else if (angle === 120) {
return "Obtuse angle";
} else if (angle === 180) {
return "Straight angle";
} else if (180 < angle < 360) {
return "Reflex angle";
}
Copy link
Contributor

Choose a reason for hiding this comment

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

  • You misunderstood the definition of these terms. Please lookup the definition of these angles and update your function accordingly.

  • You should also test the function on angle larger than or equal to 360.

// write one test at a time, and make it pass, build your solution up methodically
// just make one change at a time -- don't rush -- programmers are deep and careful thinkers
function getCardValue(card) {
let rank = card.charAt(0);
Copy link
Contributor

Choose a reason for hiding this comment

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

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

getCardValue("11♠");
getCardValue("100♠");
getCardValue("0x02♠");
getCardValue("3.1416♠");
getCardValue("0002♠");
getCardValue("2e0♠");

Comment on lines +13 to +15
test("should return true for an negative 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.

The test description is misleading because not all negative fractions are proper fraction.

For example, -7/4 is not a proper fraction.


// Case 2: Identify Improper Fractions:
test("should return false for an improper fraction", () => {
expect(isProperFraction(5, 2)).toEqual(false);
Copy link
Contributor

Choose a reason for hiding this comment

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

We can write multiple expect(isProperFraction(..., ...)).toEqual(false) with different parameters to test different improper fractions in one test to make the test more complete.

});

test("should return 10 for Face Cards", () => {
const faceCards = getCardValue("10" || "J" || "Q" || "K");
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 find out from AI what "10" || "J" || "Q" || "K" evaluates to in JS?

Note: 10 is considered a number card, not a face card.

Comment on lines +10 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.

  • The function is not expected to check if the last character is a valid suit character, so we don't have to name the suit character ("Heart").

  • 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(10);
    // Note: We could also use a loop to check all values from 2 to 10.
});

Comment on lines 7 to +33
test("should return '1st' for 1", () => {
expect(getOrdinalNumber(1)).toEqual("1st");
});

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

test("should return '3rd' for 3", () => {
expect(getOrdinalNumber(3)).toEqual("3rd");
});

test('should return "13th" for 13', () => {
expect(getOrdinalNumber(13)).toEqual("13th");
});

test("should return '41st' for 41", () => {
expect(getOrdinalNumber(41)).toEqual("41st");
});

test('should return "212th" for 212', () => {
expect(getOrdinalNumber(212)).toEqual("212th");
});

test("should return '221st' for 221", () => {
expect(getOrdinalNumber(221)).toEqual("221st");
});
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");
});

Comment on lines +2 to +4
if (count < 0) {
return "Error, negative integer";
}
Copy link
Contributor

Choose a reason for hiding this comment

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

How would the caller distinguish the result of the following two function calls?

  1. repeat("Error, negative integer", 1)
  2. repeat("", -1)

Both function calls return the same value.

Comment on lines +33 to +38
test("should throw an error if count is negative", () => {
const str = "hello";
const count = -1;
const repeatedStr = repeatStr(str, count);
expect(repeatedStr).toEqual("Error, negative integer");
});
Copy link
Contributor

Choose a reason for hiding this comment

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

If you modified repeat() to throw an error when count is negative, and you wanted to test if the function can throw an error as expected, you can use .toThrow(). You can find out more about how to use .toThrow() here: https://jestjs.io/docs/expect#tothrowerror (Note: Pay close attention to the syntax of the example)

Copy link
Contributor

Choose a reason for hiding this comment

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

Are the files in prep folder related to Sprint-3 exercise? If not, you should remove them to keep the branch clean.

@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 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Module-Structuring-And-Testing-Data The name of the module. Reviewed Volunteer to add when completing a review with trainee action still to take. 🐇 Size Small Around an hour 📅 Sprint 3 Assigned during Sprint 3 of this module

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants