London | May-2025 | Reza Jahanimir | Sprint-3 #576

Reza-Jahanimir · 2025-06-19T20:49:41Z

Learners, PR Template

Self checklist

I have committed my files one by one...
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

i tried to solve the coding changes in this repo

Questions

Ask any questions you have for your reviewer.

cjyuan · 2025-07-03T20:04:34Z

Can you format the checked boxes using the proper Markdown syntax in your PR description so that they look something like this?
Can you provide a brief description (under the "Changelist" section) summarizing the purpose of the PR and the changes you’ve made?

cjyuan · 2025-07-03T20:15:03Z

Sprint-3/2-mandatory-rewrite/1-get-angle-type.js

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


The spec isn't clear whether angle can be assigned a number not in the interval (0, 360).
When angle is >= 360, what should the function return? (Also, by definition, angles <= 0 are not considered acute angles.)

When we implement a function that can return a value, to ensure reliability, we should ensure it will always return a defined value instead of undefined (which represents "no return value").
If the parameter, angle, is not within the recognised range, we can design the function to return a special value (e.g., "Invalid angle") or throw an error.

cjyuan · 2025-07-03T20:19:57Z

Sprint-3/2-mandatory-rewrite/2-is-proper-fraction.test.js

Can your function pass the tests you specified in this script?

cjyuan · 2025-07-03T20:22:07Z

Sprint-3/2-mandatory-rewrite/2-is-proper-fraction.test.js


 // Case 4: Identify Equal Numerator and Denominator:
+test("should return false for equal numerator and denominator", () => {
+  expect(isEqualNumeratorAndDenominator(5, 5)).toEqual(false);


We could also include all combinations of +ve and -ve cases.

expect(isEqualNumeratorAndDenominator(-5, 5)).toEqual(false); expect(isEqualNumeratorAndDenominator(5, -5)).toEqual(false); expect(isEqualNumeratorAndDenominator(-5, -5)).toEqual(false);

cjyuan · 2025-07-03T20:24:36Z

Sprint-3/2-mandatory-rewrite/3-get-card-value.js

+const rank = card.slice(0, -1);
+    if (rank === "A") return 11;
+     (["K", "Q", "J", "10"].includes(rank)) return 10;
+     (!isNaN(rank) && rank >= 2 && rank <= 9) return Number(rank);


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

getCardValue("3.1416♠"); getCardValue("09♠"); getCardValue("0x02♠"); getCardValue(" 5 ♠");

cjyuan · 2025-07-03T20:28:19Z

Sprint-3/3-mandatory-practice/implement/count.test.js

+});
+
+test("should be case-sensitive", () => {
+  const str = "Millena";


Consider including some 'm' in str to illustrate the function can count 'm' but ignore 'M' in the string.

cjyuan · 2025-07-03T20:43:29Z

Sprint-3/4-stretch-investigate/Card-validator.js

+    if(cardNumber.length !==16 || isNaN(cardNumber)){
+        return false;
+    }
+    // Check if the input is exactly 16 digits and contains only numbers
+
+    let lastDigit = Number(cardNumber[cardNumber.length - 1]); 
+    if (lastDigit % 2 !== 0) {
+        return false;
+    }
+    //  the last digit must be even.
+
+
+    if (!/^\d{16}$/.test(cardNumber)) {
+        return false;
+    }


One of the if statements (lines 4-6 and lines 16-18) can be omitted.

cjyuan · 2025-07-03T20:47:02Z

Sprint-3/4-stretch-investigate/Card-validator.js

+     return true;
+     // if all tests passed ,return true which is valid card number
+


What about these two conditions?

You must have at least two different digits represented (all of the digits cannot be the same).

The sum of all the digits must be greater than 16.

cjyuan · 2025-07-03T20:48:21Z

Sprint-3/4-stretch-investigate/Card-validator.js

+console.log(isValidCreditCard("2224446668881118")); // true (valid card)
+console.log(isValidCreditCard("a92332119c011112")); // false (contains letters)
+console.log(isValidCreditCard("4444444444444444")); // true
+console.log(isValidCreditCard("333111111111110")); // false (sum less than 16)


Sum of digits in this string is more than 16.

cjyuan · 2025-07-03T20:51:43Z

Sprint-3/4-stretch-investigate/password-validator.js

+let bannedPassword = [];
 function passwordValidator(password) {


Can consider designing the function as

function passwordValidator(password, bannedPassword=[]) { ... }

cjyuan · 2025-07-03T20:54:45Z

Sprint-3/4-stretch-investigate/password-validator.test.js

The function can return false for multiple reasons.
To test a specific reason, choose an input that satisfies all other conditions except the one you're targeting. This way, if the function returns false, you can confidently attribute it to that specific condition.

For example, the function might return false for "1234" not only because it is fewer than 5 characters, but also because it lacks a lowercase letter.
As a result, we can't be certain that the function correctly handles the case of passwords shorter than 5 characters, since multiple conditions are being violated simultaneously.

A better test value could be "1$Aa" (it has all the valid characters, but its length is less than 5).

…of split using filter

cjyuan

Most of the changes look good.

Leaving responses directly in the comment threads makes tracking the discussion easier. Could you practice responding to comments directly in the comment threads?

Here is a simplified version of best practices ChatGPT suggested for responding to inline comments in a pull request:

✅ Reply to every comment – Let the reviewer know you saw it.

✏️ Make the change if needed – Fix the code if the comment points out a real issue.

🤔 Explain if you don't agree – If you think the code is fine, politely explain why.

✅ Mark as resolved when done – Only mark comments resolved after you fix or respond.

💬 Keep replies short and polite – Be respectful and to the point.

⏱️ Respond soon – Don’t wait too long to reply.

🧪 Test your changes – Make sure your fixes actually work.

📍 Reply directly under the comment – This keeps the conversation easy to follow.

cjyuan · 2025-07-04T19:57:17Z

Sprint-3/2-mandatory-rewrite/3-get-card-value.js

+  if (!/^[1-9][0-9]*$/.test(rank)) {
    throw new Error("Invalid card rank");
+  }
+  // Convert rank to a number and check if it's between 2 and 10
+  const numericRank = Number(rank);
+  if (
+    !isNaN(numericRank) &&
+    Number.isInteger(numericRank) &&
+    numericRank >= 2 &&
+    numericRank <= 10
+  ) {
+    return numericRank;
+  }


This work but the check could be much simpler; we only need to check if rank if one of 9 digits (after we have eliminated A, K, Q, J, 10).

cjyuan · 2025-07-04T20:01:37Z

Sprint-3/3-mandatory-practice/implement/get-ordinal-number.test.js

+  expect(getOrdinalNumber(1)).toEqual("1st");
+  expect(getOrdinalNumber(21)).toEqual("21st");
+  expect(getOrdinalNumber(131)).toEqual("131st");
+  expect(getOrdinalNumber(111)).toEqual("111th"); 


Numbers ending in 11, 12, 13 could have its own category.

cjyuan · 2025-07-04T20:05:43Z

Sprint-3/4-stretch-investigate/password-validator.test.js

@@ -34,8 +34,8 @@ test("password has a unknown character", () => {


 test("password has at least 5 characters", () => {
-  expect(isValidPassword("12345")).toBe(false);// Fails because no letters or special chars
-  expect(isValidPassword("1234")).toBe(false); // Fails because the password is less than 5 characters.
+  expect(isValidPassword("1£b4A")).toBe(false);// Fails because no letters or special chars


This password have 5 characters.

What does this test suppose to check?

cjyuan · 2025-07-04T20:12:12Z

Sprint-3/4-stretch-investigate/password-validator.test.js

@@ -61,7 +61,7 @@ test("valid password", () => {
 });

 test("password should not be a previously used password", () => {
-  expect(isValidPassword("Valid1!")).toBe(false); // Used before
+  expect(isValidPassword("Valid1!")).toBe(true);


Shouldn't we be testing if the function can return false when a password is one of the previous passwords?

I’ve reviewed why the test cases for reused passwords are failing, and it comes down to the design constraint of using bannedPassword = [] as a function parameter.

Since the bannedPassword list is defined as a default parameter, each time the passwordValidator function is called without explicitly passing a shared array, it creates a new empty array. This means previously used passwords aren't remembered across calls — the validator is stateless unless we manage state externally.

So in the current setup, calling passwordValidator("Valid1!") twice without passing the same bannedPassword array will always return true, because the function forgets the first password after each call.

I understand the reasoning behind wanting to pass the banned list explicitly, but just wanted to clarify that the current test fails because the function isn’t tracking state unless we provide it manually.

Correct.

The purpose of using the 2nd parameter to accept an optional array of previous passwords is that the caller can supply the previous passwords to the function if needed.

I wanted to check in because I'm feeling a bit confused about the current direction. I'm unsure whether I should revert the function back to its original form (with internal state) or keep the current version that requires the bannedPassword array to be passed in externally.

I’m also struggling to understand the core learning objective here — whether the focus is on function design, managing state, writing effective tests, or something else. I want to make sure I’m approaching this the right way and not just changing things without understanding the purpose behind them.

Could you please clarify what I should be aiming to learn from this task, and how you'd like me to proceed with the code?

The spec says the function should return false when the password is one of the previous passwords.

Your original method for handling previous passwords works (because the spec does not specify how they are supposed to be represented). I suggested a more flexible approach and you adopted it. However, doing so broke your test. So if you want to keep your current implementation (the one with 2nd parameter) , you have to fix the test. If not, you can revert the test and the implementation.

…words

cjyuan · 2025-07-05T08:12:36Z

Sprint-3/2-mandatory-rewrite/3-get-card-value.js

-    return numericRank;
-  }
+  // Digits 2–9
+  if ("23456789".includes(rank)) return Number(rank);


Almost. Any substring of "23456789" can satisfy the condition.

return the rank as a number if it's between 2 and 9
return Number(rank);

"23456789": This is a string made of all the single-digit card ranks from 2 to 9.

.includes(rank): Checks if the rank exists inside that string.
For example:
"23456789".includes("5") // → true
"23456789".includes("10") // → false

return Number(rank);
This converts the string "5" into the number 5.

"23456789".includes("45") // => true
"23456789".includes("12345") // => true

cjyuan · 2025-07-05T08:44:20Z

Sprint-3/4-stretch-investigate/password-validator.test.js

@@ -34,8 +34,8 @@ test("password has a unknown character", () => {


 test("password has at least 5 characters", () => {
-  expect(isValidPassword("12345")).toBe(false);// Fails because no letters or special chars
-  expect(isValidPassword("1234")).toBe(false); // Fails because the password is less than 5 characters.
+  expect(isValidPassword("1£b4A")).toBe(false);// Fails because no letters or special chars


What does this test suppose to check?

cjyuan · 2025-07-05T08:47:06Z

Sprint-3/4-stretch-investigate/password-validator.test.js

+  const banned = [];
+  isValidPassword("Valid1!", banned); // use it once
+  expect(isValidPassword("Valid1!", banned)).toBe(false); // second use, now rejected
+});


You could explicitly include Valid1! in banned (maybe along with a few others) to mimic a list of used password. The isValidPassword() function is not expected to remember which password it has seen before.

cjyuan

I believe you have the info you need to fix the bug in 3-get-card-value.js.

password-validator is an optional stretch exercise. You can revisit it when you have more time.

The rest of the changes are good.

Millena28 added 6 commits February 21, 2025 14:43

have done my key-implement exercise

41faaac

done the mandatory-rewrite

fb392b3

Done the mandatory-practice

52148a6

Done stretch investigate and implemented new card-validator file

4d0497e

done ALL

13220cd

made some corrections from review

396094b

Reza-Jahanimir added 📅 Sprint 3 Assigned during Sprint 3 of this module Needs Review Participant to add when requesting review labels Jun 19, 2025

cjyuan added Review in progress This review is currently being reviewed. This label will be replaced by "Reviewed" soon. and removed Needs Review Participant to add when requesting review labels Jul 3, 2025

cjyuan reviewed Jul 3, 2025

View reviewed changes

cjyuan added Reviewed Volunteer to add when completing a review and removed Review in progress This review is currently being reviewed. This label will be replaced by "Reviewed" soon. labels Jul 3, 2025

Reza-Jahanimir added 8 commits July 4, 2025 14:06

fixed the angle if statement, added validation steps

ccf0768

solve the error in testing

686efb6

solve function handling characters and validation.

435f1dd

now function differs between upper and lower characters. and instead …

cb18deb

…of split using filter

change the layout of my testing so that same test are grouped

c13ebd3

corrected the function

37f8608

omitted one the repeated statement , add validations

af01c38

redesigning the function, specify the test

2b3e360

Reza-Jahanimir added Needs Review Participant to add when requesting review and removed Reviewed Volunteer to add when completing a review labels Jul 4, 2025

cjyuan reviewed Jul 4, 2025

View reviewed changes

cjyuan added Reviewed Volunteer to add when completing a review and removed Needs Review Participant to add when requesting review labels Jul 4, 2025

Reza-Jahanimir added 3 commits July 4, 2025 23:30

made the function look simpler and cleaner

8dd332e

Numbers ending in 11, 12, 13 now are together in a test.

e80b220

i tried my best

d32bc0c

function can return false when a password is one of the previous pass…

ce330dd

…words

Reza-Jahanimir added Needs Review Participant to add when requesting review and removed Reviewed Volunteer to add when completing a review labels Jul 4, 2025

cjyuan reviewed Jul 5, 2025

View reviewed changes

cjyuan added Reviewed Volunteer to add when completing a review and removed Needs Review Participant to add when requesting review labels Jul 5, 2025

Reza-Jahanimir added 2 commits July 5, 2025 14:03

add a line of explanation

c969da7

change the function the its original, cleaned the test syntax

95c85ba

Reza-Jahanimir added Needs Review Participant to add when requesting review and removed Reviewed Volunteer to add when completing a review labels Jul 5, 2025

cjyuan reviewed Jul 5, 2025

View reviewed changes

cjyuan added Complete Volunteer to add when work is complete and review comments have been addressed and removed Needs Review Participant to add when requesting review labels Jul 5, 2025

Reza-Jahanimir mentioned this pull request Jul 5, 2025

London | May-2025 | Structuring and testing data | Reza Jahanimir Reza-Jahanimir/My-Coursework-Planner#104

Open

		return true;
		// if all tests passed ,return true which is valid card number

		let bannedPassword = [];
		function passwordValidator(password) {

Uh oh!

London | May-2025 | Reza Jahanimir | Sprint-3 #576

Are you sure you want to change the base?

London | May-2025 | Reza Jahanimir | Sprint-3 #576

Uh oh!

Conversation

Reza-Jahanimir commented Jun 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Learners, PR Template

Changelist

Questions

Uh oh!

cjyuan commented Jul 3, 2025

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!

cjyuan left a comment • 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!

cjyuan Jul 4, 2025 • 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!

cjyuan Jul 4, 2025 • 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!

cjyuan Jul 4, 2025 • 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!

Reza-Jahanimir Jul 5, 2025 • 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!

cjyuan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reza-Jahanimir commented Jun 19, 2025 •

edited

Loading

cjyuan left a comment •

edited

Loading

cjyuan Jul 4, 2025 •

edited

Loading

cjyuan Jul 4, 2025 •

edited

Loading

cjyuan Jul 4, 2025 •

edited

Loading

Reza-Jahanimir Jul 5, 2025 •

edited

Loading