Skip to content

WESTMIDLANDS | ITP-MAY-25 | AHMAD EHSAS | MODULE STRUCTURING AND TESTING DATA SPRINT-3 #656

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 25 commits into
base: main
Choose a base branch
from

Conversation

ahmadehsas
Copy link

@ahmadehsas ahmadehsas commented Jul 12, 2025

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

Briefly explain your PR.

Completed sprint-3, dealing with errors and understanding the flow of a program

Questions

@ahmadehsas ahmadehsas added the Needs Review Participant to add when requesting review label Jul 12, 2025
@cjyuan cjyuan added the Review in progress This review is currently being reviewed. This label will be replaced by "Reviewed" soon. label Jul 17, 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.

Can you revert the changes made to these two files (see pic) in Sprint-1 folder? They are not related to Sprint-3 exercise.
image

if (rank === "A") return 11;
if (["K", "Q", "J"].includes(rank)) return 10;

const number = parseInt(rank);
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("0x02♠");
getCardValue("2.1♠");
getCardValue("0002♠");
getCardValue("3ABC♠");

Consider looking up how parseInt() work.

Copy link
Author

Choose a reason for hiding this comment

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

Yes, the function returns the card value based on the card's rank.

Copy link
Author

Choose a reason for hiding this comment

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

Yes, the function returns a value based on the card's rank condition.
Screenshot 2025-07-20 6 08 25 PM
Screenshot 2025-07-20 6 05 31 PM
I've attached the screenshots.

Copy link
Contributor

Choose a reason for hiding this comment

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

parseInt() can return an integer but do you consider the values I suggested valid?

Copy link
Author

Choose a reason for hiding this comment

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

As parseInt() returns Integers, and the values with decimal numbers can only return an integer, not the decimal part. So technically, the values you suggested are valid in the sense that it doesn't cause an error. But it does not reflect the true value of the number. It truncates the decimal.

Copy link
Contributor

Choose a reason for hiding this comment

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

The objective of the function is to determine the value of the rank of a card.

If you were to design a card game, would you allow the card to be displayed as
"2.9999999999999999999999♠", "3AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA♠"?

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

test("should return 10 for 10 of Diamonds", () => {
const tenofDiamonds = getCardValue("10♦");
expect(tenofDiamonds).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.

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.
});

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 make any change to your code accordingly.

Comment on lines 18 to 31
test("should return 10 for Jack of Clubs", () => {
const jackofClubs = getCardValue("J♣");
expect(jackofClubs).toEqual(10);
});

test("should return 10 for Queen of Hearts", () => {
const queenofHearts = getCardValue("Q♥");
expect(queenofHearts).toEqual(10);
});

test("should return 10 for King of Spades", () => {
const kingofSpades = getCardValue("K♠");
expect(kingofSpades).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 combine these tests and place them under the category "should return 10 for face cards (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.

The tests in this Jest script are incomplete. When you have fully implemented the function in Sprint-3/3-mandatory-practice/implement/get-ordinal-number.js, can you prepare tests in this script to cover all valid integers in the following way:

Group all possible input values into meaningful categories, and 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");
});

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 update the tests in Sprint-3/3-mandatory-practice/implement/repeat.test.js to check your implementation?

Copy link
Author

Choose a reason for hiding this comment

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

The test is updated, and all the tests have been passed.

Copy link
Contributor

Choose a reason for hiding this comment

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

I still could not see an updated Sprint-3/3-mandatory-practice/implement/repeat.test.js file. Did you forget to push your changes to GitHub?

Copy link
Author

Choose a reason for hiding this comment

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

My apologies, the tests are now updated.

@cjyuan cjyuan added Reviewed Volunteer to add when completing a review and removed Needs Review Participant to add when requesting review Review in progress This review is currently being reviewed. This label will be replaced by "Reviewed" soon. labels Jul 17, 2025
@ahmadehsas ahmadehsas added Needs Review Participant to add when requesting review and removed Reviewed Volunteer to add when completing a review labels Jul 20, 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.

Files in "Sprint-1" folder not yet reverted.

if (rank === "A") return 11;
if (["K", "Q", "J"].includes(rank)) return 10;

const number = parseInt(rank);
Copy link
Contributor

Choose a reason for hiding this comment

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

parseInt() can return an integer but do you consider the values I suggested valid?

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

test("should return 10 for 10 of Diamonds", () => {
const tenofDiamonds = getCardValue("10♦");
expect(tenofDiamonds).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.

Why not make any change to your code accordingly.

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 update the tests in this script so that they cover all valid integers?

Copy link
Contributor

Choose a reason for hiding this comment

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

I still could not see an updated Sprint-3/3-mandatory-practice/implement/repeat.test.js file. Did you forget to push your changes to GitHub?

@cjyuan cjyuan removed the Needs Review Participant to add when requesting review label Jul 20, 2025
@cjyuan cjyuan added the Reviewed Volunteer to add when completing a review label Jul 20, 2025
@ahmadehsas ahmadehsas added Needs Review Participant to add when requesting review and removed Reviewed Volunteer to add when completing a review labels Jul 21, 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.

Deleted files are still considered "changed files", and they will still show up in the "Files changed" tab, making code reviewing inconvenience. (In practice, it would also make merging difficult.)

image

One way to revert the changes is to do the following (to restore the files to their original version):

  1. Download your main branch as a ZIP file.

  2. In VSCode, switch to this branch (coursework/sprint-3).

  3. Copy these two files from the ZIP file to this branch (make sure they are copied to the right folder).

  • Sprint-1/1-key-exercises/1-count.js
  • Sprint-1/readme.md
  1. Verify and commit the change.

  2. Push the change to Github.

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

test("append 'st' to numbers ending in 1, except those ending in 11", () => {
expect(getOrdinalNumber(1)).toEqual("1st");
expect(getOrdinalNumber(21)).toEqual("21st");
expect(getOrdinalNumber(121)).toEqual("121st");
});
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 update the tests in this script so that they cover all valid integers?

if (rank === "A") return 11;
if (["K", "Q", "J"].includes(rank)) return 10;

const number = parseInt(rank);
Copy link
Contributor

Choose a reason for hiding this comment

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

The objective of the function is to determine the value of the rank of a card.

If you were to design a card game, would you allow the card to be displayed as
"2.9999999999999999999999♠", "3AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA♠"?

@cjyuan cjyuan added Reviewed Volunteer to add when completing a review and removed Needs Review Participant to add when requesting review labels Jul 21, 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants