-
-
Notifications
You must be signed in to change notification settings - Fork 195
London | May-2025 | Reza Jahanimir | Sprint-3 #576
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
base: main
Are you sure you want to change the base?
London | May-2025 | Reza Jahanimir | Sprint-3 #576
Conversation
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"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can your function pass the tests you specified in this script?
|
||
// Case 4: Identify Equal Numerator and Denominator: | ||
test("should return false for equal numerator and denominator", () => { | ||
expect(isEqualNumeratorAndDenominator(5, 5)).toEqual(false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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);
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); |
There was a problem hiding this comment.
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("3.1416♠");
getCardValue("09♠");
getCardValue("0x02♠");
getCardValue(" 5 ♠");
}); | ||
|
||
test("should be case-sensitive", () => { | ||
const str = "Millena"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider including some 'm'
in str
to illustrate the function can count 'm' but ignore 'M' in the string.
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; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One of the if statements (lines 4-6 and lines 16-18) can be omitted.
return true; | ||
// if all tests passed ,return true which is valid card number | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sum of digits in this string is more than 16.
let bannedPassword = []; | ||
function passwordValidator(password) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can consider designing the function as
function passwordValidator(password, bannedPassword=[]) {
...
}
expect(isValidPassword("1234")).toBe(false); // Fails because the password is less than 5 characters. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
expect(getOrdinalNumber(1)).toEqual("1st"); | ||
expect(getOrdinalNumber(21)).toEqual("21st"); | ||
expect(getOrdinalNumber(131)).toEqual("131st"); | ||
expect(getOrdinalNumber(111)).toEqual("111th"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Numbers ending in 11, 12, 13 could have its own category.
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This password have 5 characters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this test suppose to check?
@@ -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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we be testing if the function can return false
when a password is one of the previous passwords?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
return numericRank; | ||
} | ||
// Digits 2–9 | ||
if ("23456789".includes(rank)) return Number(rank); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost. Any substring of "23456789" can satisfy the condition.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"23456789".includes("45") // => true
"23456789".includes("12345") // => true
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this test suppose to check?
const banned = []; | ||
isValidPassword("Valid1!", banned); // use it once | ||
expect(isValidPassword("Valid1!", banned)).toBe(false); // second use, now rejected | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Learners, PR Template
Self checklist
Changelist
i tried to solve the coding changes in this repo
Questions
Ask any questions you have for your reviewer.