London | 25-ITP-Sep | Adnaan Abo | Sprint 3 | coursework/sprint-3-implement-and-rewrite #713

AdnaanA · 2025-09-26T09:49:16Z

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

completed the coursework/sprint-3 implement-and-rewrite by answering the questions in the tasks and implementing test-cases and making sure all test-cases have passed.

…jest/2-is-proper-fraction.test.js modified: Sprint-3/1-implement-and-rewrite-tests/rewrite-tests-with-jest/3-get-card-value.test.js modified: Sprint-3/2-practice-tdd/count.test.js

illicitonion

This is generally looking good, but I left a few things to look at

Sprint-3/1-implement-and-rewrite-tests/implement/2-is-proper-fraction.js

illicitonion · 2025-10-01T17:37:17Z

Sprint-3/1-implement-and-rewrite-tests/implement/3-get-card-value.js

+  } else if (!isNaN(rank) && Number(rank) >= 2 && Number(rank) <= 9) {
+    return Number(rank);
+  } else {
+    throw new Error("Invalid card rank");


It can be useful to include what the problematic data was in an error message, so that someone knows what they should be debugging - can you give that a go?

Yes you are correct it would be helpful/useful to include the problematic data in an error message and we can achieve this by doing.

throw new Error('Invalid card rank: "$(rank)"');

illicitonion · 2025-10-01T17:38:05Z

Sprint-3/1-implement-and-rewrite-tests/implement/3-get-card-value.js

+  } else if (["K", "Q", "J", "10"].includes(rank)) {
+    return 10;
+  } else if (!isNaN(rank) && Number(rank) >= 2 && Number(rank) <= 9) {


You could have handled "10" in either the K/Q/J case, or the 2-9 case - which do you prefer? What advantages/disadvantages do you think each has?

hello,

Thank you for the feedback, I prefer to handle the "10" in the K/Q/J case. for clarity and explicitness, and to maintain semantical grouping since "10" has the same card value as the face cards. a disadvantage could be mixing string handling logic "K/Q/J" with numeric checking logic "10".

We can also handle "10" in the 2-9 case and the pros for this would be logical consistency the numeric branch handles anything numeric between 2-10. and its easier to extend if we ever want to adjust the ranges to include 1 or 11 or validate different ranges we just change the numeric branch. Disadvantages risk of mis-parsing since !isNaN(rank) is loose we might accidently accept strings like "10abc". Edge-case handling we need to make sure to handle trimming, ect. in case someone passes weird strings like "010" and " 10 ".

Sounds good, thanks for sharing your thoughts!

Personally I prefer 10 to be included in the number case, because it leads to simpler code. In the numeric case, we already need an upper bound, so the difference between Number(rank) <= 9 and Number(rank) <= 10 is really small, whereas adding a whole extra element to the array we're checking is adding an extra piece: ["K", "Q", "J", "10"] vs ["K", "Q", "J"].

But you're right that there's not one obviously better answer - there are advantages and disadvantages to each :)

Sprint-3/1-implement-and-rewrite-tests/implement/3-get-card-value.js

illicitonion · 2025-10-01T17:40:43Z

Sprint-3/1-implement-and-rewrite-tests/rewrite-tests-with-jest/3-get-card-value.test.js

+  expect(getCardValue("A♥")).toEqual(11);
+}); 
 // Case 5: Handle Invalid Cards:
+test("should return null for invalid cards", () => {


This test looks like it's expecting something different from your previous test - before it was checking it throws, here it's checking for null. Does the same code pass both tests? Should the tests look more similar?

I have modified the case 5: test to check it throws error rather than checking for null. I have also run a test with pass.

illicitonion

Looks good! Just one thing to fix up and this is done!

illicitonion · 2025-10-02T15:18:44Z

Sprint-3/1-implement-and-rewrite-tests/implement/3-get-card-value.js

+  console.assert(false, "Expected an error to be thrown for invalid card rank");
+} catch (e) {
+  console.assert(
+    e.message ===  'Invalid card rank: "${rank}"',


There's a tension in testing between calculating expected values in tests (to avoid duplication), or hard-coding expected values (to make it really obvious that the test is correct).

Can you change this test so that it actually includes "1" in the expected error message:

Suggested change

e.message === 'Invalid card rank: "${rank}"',

e.message === 'Invalid card rank: "1"',

I think you'll find that the test is actually failing, so we should fix the code :)

Thank for the feedback and I have now included "1" in the expected error message. I have run the test with a pass.

thank you for the suggested change.

I just ran the test and it failed...

% node Sprint-3/1-implement-and-rewrite-tests/implement/3-get-card-value.js Assertion failed: Expected error message to be "Invalid card rank" but got "Invalid card rank: "${rank}""

Can you check again?

(Also, your Jest tests wouldn't catch this same bug, can you fix them up so they do as well?)

Thank you for picking on that and being patient with me. test was not failing on my side because Jest’s .toThrow("Invalid card rank") only checks that the error message contains that substring, not that it matches exactly. now I have updated the code and it passes as it should.

illicitonion · 2025-10-02T15:21:17Z

Sprint-3/1-implement-and-rewrite-tests/implement/3-get-card-value.js

+  } else if (["K", "Q", "J", "10"].includes(rank)) {
+    return 10;
+  } else if (!isNaN(rank) && Number(rank) >= 2 && Number(rank) <= 9) {


Sounds good, thanks for sharing your thoughts!

Personally I prefer 10 to be included in the number case, because it leads to simpler code. In the numeric case, we already need an upper bound, so the difference between Number(rank) <= 9 and Number(rank) <= 10 is really small, whereas adding a whole extra element to the array we're checking is adding an extra piece: ["K", "Q", "J", "10"] vs ["K", "Q", "J"].

But you're right that there's not one obviously better answer - there are advantages and disadvantages to each :)

illicitonion

Looks great, thank you!

AdnaanA added 6 commits September 26, 2025 10:08

implemented test cases and test passed

3cbfc96

created test cases for fractions and tested them

61f0da7

created testcases for get card value and tested with a pass

f6e6f38

created testcases for get angle-type and tested to pass

6fe6458

created testcases for proper-fractions, cases tested and passed.

b07437b

test-cases written and passed for get card value.

6152bda

AdnaanA added Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. 📅 Sprint 3 Assigned during Sprint 3 of this module labels Sep 26, 2025

AdnaanA added 2 commits September 26, 2025 10:57

modified: Sprint-3/1-implement-and-rewrite-tests/rewrite-tests-with-…

d8e8c60

…jest/2-is-proper-fraction.test.js modified: Sprint-3/1-implement-and-rewrite-tests/rewrite-tests-with-jest/3-get-card-value.test.js modified: Sprint-3/2-practice-tdd/count.test.js

modified: Sprint-3/2-practice-tdd/count.test.js

48c7af8

illicitonion 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 1, 2025

illicitonion reviewed Oct 1, 2025

View reviewed changes

illicitonion 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 1, 2025

AdnaanA added 3 commits October 2, 2025 07:16

updated and simplified the isproperfraction function

7f195fc

changed the custom error class to embed the invalid card rank

db40788

modified case 5 handle invalid cards

91ced4e

AdnaanA added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Oct 2, 2025

illicitonion reviewed Oct 2, 2025

View reviewed changes

illicitonion removed the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Oct 2, 2025

changed the expected error message to include "1"

991180d

AdnaanA added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Oct 2, 2025

illicitonion removed the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Oct 3, 2025

test case fixed and test passed

d6b9753

AdnaanA added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Oct 3, 2025

illicitonion reviewed Oct 6, 2025

View reviewed changes

AdnaanA mentioned this pull request Oct 28, 2025

ITP-SEP-25 | Structuring and testing data | Adnaan Abo AdnaanA/My-Coursework-Planner#70

Open

	e.message === 'Invalid card rank: "${rank}"',
	e.message === 'Invalid card rank: "1"',

Uh oh!

London | 25-ITP-Sep | Adnaan Abo | Sprint 3 | coursework/sprint-3-implement-and-rewrite #713

Are you sure you want to change the base?

London | 25-ITP-Sep | Adnaan Abo | Sprint 3 | coursework/sprint-3-implement-and-rewrite #713

Uh oh!

Conversation

AdnaanA commented Sep 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

illicitonion left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

illicitonion left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

illicitonion left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

AdnaanA commented Sep 26, 2025 •

edited

Loading