London | JAN-2026 ITP | Asha Ahmed | Sprint 3 | implement and rewrite tests coursework by ashaahmed7 · Pull Request #1035 · CodeYourFuture/Module-Structuring-and-Testing-Data

ashaahmed7 · 2026-02-23T21:59:20Z

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

Implemented getAngleType, isProperFraction, and getCardValue solutions.

Rewrote tests using Jest to cover expected cases, including boundary/invalid inputs.

Questions

n/a

…ion cases

… card cases

…and invalid angles

…o, negative, decimal, and large numbers

…ce cards, and Invalid cards

netEmmanuel · 2026-03-05T19:26:09Z

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

@@ -50,3 +64,33 @@ try {
 } catch (e) {}


The catch block can be improved. it's currently empty

You are referring to the original CodeYourFuture code example, which I did not write. The question I was asked was “What other invalid card cases can you think of?” (see below), not to modify the code you are commenting on.

netEmmanuel · 2026-03-05T19:29:36Z

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


 // What other invalid card cases can you think of?
+
+try {


An empty catch block is not good practice.

Fix the catch blocks in this file

My answers are based on the example given above.

Avoid empty catch. At the very least, there should be a comment in the empty block explaining why you're swallowing the exception at that point or output the exception.

Look up the standard on how to write a try and catch block; it would help fix the syntax.

I'm still learning JavaScript, so I’ll look into catch blocks as I’m not entirely sure what you mean yet. My understanding was that the code is technically correct, and my answers were based on the example provided by CodeYourFuture above. Given that, it feels like a big change to fix the file itself rather than respond to the questions asked.

Hey Asha, so Emmanuel is actually right here. We do want that "catch" block to be more informative.

Let's think of it another way. The goal for this test is to show that when an invalid card is passed in, an error is thrown. That's a really interesting problem because when you pass a invalid card into the function, an error will throw and you won't have the chance to "show" that the error behaviour is tested for because the whole programme crashes. Annoying.

But that's why try...catch is so useful. It lets us "try" code in the try... block which might fail, and if an error occurs we can "catch" the error in the catch... block without crashing the programme 🎉

The next step which Emmanuel is referencing is that it isn't enough to just have a empty catch block we should use that block to communicate that the expected behaviour has occurred. In this case, the expected behaviour is the error message is thrown. So that catch block has a parameter e
E.g. catch (e) {} -> can you use this parameter to console log something that SHOWS us that the right error message was thrown?

More helpful reading about catch blocks:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/try...catch
https://www.w3schools.com/js/js_errors.asp
https://dev.to/wisdomudo/javascript-trycatch-explained-a-beginners-guide-to-error-handling-2kf6

hint: What could that e parameter represent? Docs and logging could help here

Edit: this is really a nice-to-have but not an expectation based on how this task is designed. We could argue the console log on line 71 not running is enough evidence in this task that the error was thrown.

netEmmanuel · 2026-03-05T19:30:56Z

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

 // Case 1: Ace (A)
 test(`Should return 11 when given an ace card`, () => {
  expect(getCardValue("A♠")).toEqual(11);
+  expect(getCardValue("A♣")).toEqual(11);


These are duplicated test lines. Is there a reason for this?

I don't think this is a duplication. We have two codes who have different purpose: one for spades and one for clubs. Can you please check again?

Fair to say it is not a duplication here as they are different suites

Poonam-raj

So I've done a quick review of what's happening here.

Few things I love

choices of methods and compact if statements to ensure logic is DRY.
test cases and detail in the jest testing
error throwing

Few areas to think about (but are not core criteria here):

else if vs if statements, when do we need a else if?
how big we make our test blocks, what behaviour do we ACTUALLY care about

Poonam-raj · 2026-03-11T11:51:59Z

Sprint-3/1-implement-and-rewrite-tests/implement/1-get-angle-type.js

+console.assert(getAngleType(45) === "Acute angle");
+console.assert(getAngleType(90) === "Right angle");
+console.assert(getAngleType(120) === "Obtuse angle");
+console.assert(getAngleType(180) === "Straight angle");
+console.assert(getAngleType(270) === "Reflex angle");
+console.assert(getAngleType(0) === "Invalid angle");
+console.assert(getAngleType(360) === "Invalid angle");
+


In future I'd remove console.asserts as they aren't adding anything to your solution thanks to your actual assertions below on line 60 onwards. Before you submit code make sure you clear those less useful bits like these lines

Poonam-raj · 2026-03-11T11:53:34Z

Sprint-3/1-implement-and-rewrite-tests/implement/1-get-angle-type.js

+  if (angle > 0 && angle < 90) {
+    return "Acute angle";
+  } else if (angle === 90) {
+    return "Right angle";
+  } else if (angle > 90 && angle < 180) {
+    return "Obtuse angle";
+  } else if (angle === 180) {
+    return "Straight angle";
+  } else if (angle > 180 && angle < 360) {
+    return "Reflex angle";
+  } else {
+    return "Invalid angle";
+  }


great work here - an opportunity for refactor is considering if every if block contains a return statement do we need else if blocks on lines 20 -26 or can they be just if blocks? Try it out, see if you can predict the outcome and whether the code runs like you expect.

Poonam-raj · 2026-03-11T11:54:23Z

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

+  if (denominator <= 0) {
+    return false;
+  }
+  if (numerator < denominator && numerator >= 0) {
+    return true;
+  }


This is a great example of what i was suggesting in my previous comment! That a plain if block works here, no else if needed. Why is that the case?

Poonam-raj · 2026-03-11T11:55:26Z

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

+  if (denominator <= 0) {
+    return false;
+  }


You return false on line 21 and you return false here. Can you refactor this function completely to be even more efficient about returning false? Can we return false just once in the solution?

Poonam-raj · 2026-03-11T11:56:27Z

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


 function getCardValue(card) {
  // TODO: Implement this function
+  if (!["♠", "♣", "♦", "♥"].includes(card.slice(-1)))


Great use of methods here (includes, slice) and bang operator (!), how did you feel about nesting these methods like this?

Poonam-raj · 2026-03-11T11:58:41Z

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


 // What other invalid card cases can you think of?
+
+try {


Edit: this is really a nice-to-have but not an expectation based on how this task is designed. We could argue the console log on line 71 not running is enough evidence in this task that the error was thrown.

Poonam-raj · 2026-03-11T11:59:20Z

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

+try {
+  getCardValue("");
+
+  console.log("Error was not thrown for invalid card");
+} catch (error) {}
+
+try {
+  getCardValue("ABC");
+
+  console.log("Error was not thrown for invalid card");
+} catch (error) {}
+
+try {
+  getCardValue("A");
+
+  console.log("Error was not thrown for invalid card");
+} catch (error) {}
+
+try {
+  getCardValue("JK");
+
+  console.log("Error was not thrown for invalid card");
+} catch (error) {}


Super repeated code (breaks the Don't Repeat Yourself rule). Could you refactor this code into a reusable function that you pass your invalid cases to?

Poonam-raj · 2026-03-11T11:59:48Z

Sprint-3/1-implement-and-rewrite-tests/rewrite-tests-with-jest/1-get-angle-type.test.js

+  expect(getAngleType(181)).toEqual("Reflex angle");
+  expect(getAngleType(280)).toEqual("Reflex angle");
+  expect(getAngleType(359)).toEqual("Reflex angle");
+});


Across this file, good grouping of similar behaviours.

Poonam-raj · 2026-03-11T12:04:21Z

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

+});
+
+// Case 2: Handle Number Cards (2-10):
+test("should return the appropriate number from 2 to 10", () => {


I would say this is an extremely excessive jest test block. I'd prefer to take a snapshot of the test cases that we need to check this behaviour for. Doing every combo of number and suite is very detailed but also quite bulky.
One reason this is a bad idea is it could be computationally heavy, e.g. if the function was running a very complicated algorithm for this behaviour.
But the tradeoff is that you know it works for every case.

I would argue that the "suite" doesn't actually matter in the case described; the number is important. So you should focus on this. If the suite mattered, the test case would be including it in the description but it only mentioned number cards.

Poonam-raj · 2026-03-11T12:04:39Z

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

+  expect(() => getCardValue("KJ")).toThrow("Invalid card rank.");
+  expect(() => getCardValue("AK")).toThrow("Invalid card rank.");
+  expect(() => getCardValue("  ")).toThrow("Invalid card rank.");
+  expect(() => getCardValue("S♠")).toThrow("Invalid card rank.");
+  expect(() => getCardValue("J♠♠")).toThrow("Invalid card rank.");


ashaahmed7 added 7 commits February 21, 2026 15:57

Implement getAngleType function and added tests for all angle types

b150432

Implement isProperFraction function and added tests for various fract…

bec514f

…ion cases

Implement getCardValue function and added tests for valid and invalid…

72a8a7d

… card cases

Add tests for angle types including right, obtuse, straight, reflex, …

5fa7695

…and invalid angles

Added tests for isProperFraction covering various cases including zer…

7e4cb27

…o, negative, decimal, and large numbers

Added tests for getCardValue function covering Aces, Number cards, Fa…

56c990b

…ce cards, and Invalid cards

renamed brach

ad8efb8

ashaahmed7 added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Feb 23, 2026

ashaahmed7 mentioned this pull request Feb 24, 2026

JAN 26 | Structuring and testing data | Asha Ahmed ashaahmed7/My-Coursework-Planner#21

Open

netEmmanuel suggested changes Mar 5, 2026

View reviewed changes

netEmmanuel 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 Mar 5, 2026

ashaahmed7 added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Mar 6, 2026

ashaahmed7 requested a review from netEmmanuel March 6, 2026 16:23

Poonam-raj reviewed Mar 11, 2026

View reviewed changes

Poonam-raj added Complete Volunteer to add when work is complete and all review comments have been addressed. and removed Review in progress This review is currently being reviewed. This label will be replaced by "Reviewed" soon. labels Mar 11, 2026

Uh oh!

Conversation

ashaahmed7 commented Feb 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Learners, PR Template

Changelist

Questions

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!

netEmmanuel Mar 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Poonam-raj Mar 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Choose a reason for hiding this comment

Uh oh!

Poonam-raj 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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ashaahmed7 commented Feb 23, 2026 •

edited

Loading

netEmmanuel Mar 6, 2026 •

edited

Loading

Poonam-raj Mar 11, 2026 •

edited

Loading