London | 26-ITP-May | Dipa Sarker | Sprint 3 | Implement and rewrite final by Dipa-Sarker · Pull Request #1470 · CodeYourFuture/Module-Structuring-and-Testing-Data

Dipa-Sarker · 2026-07-06T10:41:03Z

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 the required functionality for the angle type, proper fraction and card value logic.
Rewrote and expanded the tests using Jest.
Added test cases to verify expected behavior and improve test coverage.
Refactored the implementation where needed to ensure the code passes all Jest tests.

Dipa-Sarker · 2026-07-06T10:45:57Z

Hello Reviewers,
I just wanted to inform that, I made several mistakes creating branching for each of the sprints (Sprint-1, Sprint-2, Sprint-3/implement and re-write tests, Sprint-3/Practice-tdd. After so many attempts I was able to create separate branches and for that reason I named this branch "implement and re-write final". I hope I will more conscious about this issue in near future. Thank you.

Luro91 · 2026-07-10T04:34:38Z

+  if (angle <= 0 || angle >= 360) {
+    return "Invalid angle";
+  }
+  else if (angle == 90) {


Whats the difference between == and === and why are you using == here?

== converts and compares operands that are of different types and === always considers operands of different types to be different. In this case, angle == 90 which means LHS is compared with RHS and it is equal. I can also use ===.

Luro91 · 2026-07-10T04:36:02Z

+//module.exports = getAngleType;
+
+// This helper function is written to make our assertions easier to read.
+// If the actual output matches the target output, the test will pass
+// function assertEquals(actualOutput, targetOutput) {
+  //console.assert(
+    //actualOutput === targetOutput,
+    //`Expected ${actualOutput} to equal ${targetOutput}`
+  //);
+//}
+
+// TODO: Write tests to cover all cases, including boundary and invalid cases.
+// Example: Identify Right Angles
+//const right = getAngleType(90);
+//assertEquals(right, "Right angle");


Why is there commented out code?

In my file there is no commented code showing. I have attached the screenshot.

Did you commit and push all code?
If you check on the remote (for example in the PR diff view on Github) you' ll see the code: https://github.com/CodeYourFuture/Module-Structuring-and-Testing-Data/pull/1470/changes#diff-714a296fc0e0b2cf1e44da9dcbd08ccd678295a69af1884e7f2e1686aaef8626R81

Luro91 · 2026-07-10T04:36:44Z

+  expect(getAngleType(91)).toEqual("Obtuse angle");
+  expect(getAngleType(120)).toEqual("Obtuse angle");
+  expect(getAngleType(179)).toEqual("Obtuse angle");
+});
+test(`should return "Straight angle" when (0 == 180)`, () => {
+  expect(getAngleType(180)).toEqual("Straight angle");
+});
+test(`should return "Reflex angle" when (180 < angle < 360)`, () => {
+  expect(getAngleType(181)).toEqual("Reflex angle");
+  expect(getAngleType(270)).toEqual("Reflex angle");
+  expect(getAngleType(359)).toEqual("Reflex angle");


Good job on testing the border cases here

Luro91 · 2026-07-10T04:37:16Z

+  expect(getAngleType(-1)).toEqual("Invalid angle");
+  expect(getAngleType(370)).toEqual("Invalid angle");


Which border cases can you test here? (Which numbers are closest to the condition?)

I tested for the invalid case when (angle <= 0 || angle >= 360). So, when i considered -1 and 370, the closest number for this condition is -1 and 361. Because -1 is closest to 0 and 361 is closest to 360.

Luro91 · 2026-07-10T04:40:58Z

+test(`should return false when numerator is negative`, () => {
+  expect(isProperFraction(-1, 1)).toEqual(false);
+});
+test(`should return false when denominator is negative`, () => {
+  expect(isProperFraction(1, -1)).toEqual(false);
+});


What about values like -1/2 or 1/-2. What should the function return for them?

I think for values like -1/2 or 1/-2, the function will also return false because proper fractions normally require positive numerator and denominator.

I believe a proper fraction can also include negative values
In general, a common fraction is said to be a proper fraction if the absolute value of the fraction is strictly less than one—that is, if the fraction is greater than −1 and less than 1.

Luro91 · 2026-07-10T04:42:38Z


 function isProperFraction(numerator, denominator) {
-  // TODO: Implement this function
+  if (numerator <= 0 || denominator <= 0) {


Why does the numerator be bigger than 0?

Sorry I cannot understand your question. It says, the numerator is less than or equal to 0.

Sorry I was missing a word. Why can the numerator not be 0?

Luro91 · 2026-07-10T04:43:43Z

+ if (!validRank.includes(rank) || !validSuit.includes(suit)) {
+  throw new Error("Invalid Card");
+  }
+
+  if (rank === "A") {
+    return 11;
+  }
+  else if (rank === "J" || rank=== "Q" || rank === "K") {
+    return 10;
+  }

-// The line below allows us to load the getCardValue function into tests in other files.
-// This will be useful in the "rewrite tests with jest" step.
-module.exports = getCardValue;
+ else if (rank >= 2 && rank <= 10) {
+  return Number(rank);
+  } 


The indentation is a bit off. How can you ensure consistent formatting in your code?

I used prettier tool to fix the formation of the entire code. Please let me know if it is okay or not.

I don't see any changes on this file. Did you push the changes?

Luro91 · 2026-07-10T04:44:14Z

+  const validRank = ["A","2","3","4","5","6","7","8","9","10","J","Q","K"];
+  const validSuit = ["♠","♥","♦","♣"];


Good job on explicitly listing all allowed ranks and suits. This makes the code easier to read and the logic for checking easier as well.

Luro91 · 2026-07-10T04:44:38Z

-// The line below allows us to load the getCardValue function into tests in other files.
-// This will be useful in the "rewrite tests with jest" step.
-module.exports = getCardValue;
+ else if (rank >= 2 && rank <= 10) {


What happens if the condition is false?

If rank >= 2 && rank <= 10 is false, suppose if rank = 11, the function does not return Number(rank). For invalid rank throw new Error("Invalid Card") check before reaching this point.

So when the code reaches this point you can already be certain that the rank is between 2 and 10. You did the checks earlier so this condition is not needed and can be removed.

Luro91 · 2026-07-10T04:46:16Z

+  expect(getCardValue("K♦")).toEqual(10);
+ });
+});
+ describe("Invalid Cards", () => {


Which part of the condition is not covered by the invalid test cases yet?
if (!validRank.includes(rank) || !validSuit.includes(suit)) {

Is it for Invalid suit?

Yes exactly. Please add some test cases for verifying teh valid suit check works correctly

Luro91 · 2026-07-13T04:28:02Z

It seems some of your changes are not pushed. Please always ensure the changes are pushed correctly by checking the diff view of the PR in Github directly

Dipa-Sarker added 4 commits June 29, 2026 15:16

Sprint-3 implementation added

40e0672

rewrite get-angle-type-with-jest

3b832f3

fix conditions for proper fraction

d0c84ec

rewrite get-card-value with jest

d469d94

Dipa-Sarker added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Jul 6, 2026

Luro91 reviewed Jul 10, 2026

View reviewed changes

Luro91 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 Jul 10, 2026

made correction in tests

8fe3c92

Dipa-Sarker added Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. and removed Reviewed Volunteer to add when completing a review with trainee action still to take. labels Jul 13, 2026

Luro91 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 Jul 13, 2026

		expect(getAngleType(-1)).toEqual("Invalid angle");
		expect(getAngleType(370)).toEqual("Invalid angle");

		const validRank = ["A","2","3","4","5","6","7","8","9","10","J","Q","K"];
		const validSuit = ["♠","♥","♦","♣"];

Uh oh!

Uh oh!

Conversation

Dipa-Sarker commented Jul 6, 2026

Learners, PR Template

Changelist

Uh oh!

Dipa-Sarker commented Jul 6, 2026

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!

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!

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!

Luro91 commented Jul 13, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants