Skip to content

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

Open
wants to merge 20 commits into
base: main
Choose a base branch
from

Conversation

Reza-Jahanimir
Copy link

@Reza-Jahanimir Reza-Jahanimir commented Jun 19, 2025

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.

@Reza-Jahanimir 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 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
Copy link
Contributor

cjyuan commented Jul 3, 2025

image

  1. Can you format the checked boxes using the proper Markdown syntax in your PR description so that they look something like this?
    image

  2. Can you provide a brief description (under the "Changelist" section) summarizing the purpose of the PR and the changes you’ve made?

Comment on lines 3 to 6
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";
Copy link
Contributor

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.

Copy link
Contributor

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);
Copy link
Contributor

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);
Copy link
Contributor

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";
Copy link
Contributor

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.

Comment on lines 4 to 18
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;
}
Copy link
Contributor

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.

Comment on lines 21 to 23
return true;
// if all tests passed ,return true which is valid card number

Copy link
Contributor

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)
Copy link
Contributor

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.

Comment on lines 1 to 2
let bannedPassword = [];
function passwordValidator(password) {
Copy link
Contributor

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=[]) {
  ...
}

Comment on lines 37 to 38
expect(isValidPassword("1234")).toBe(false); // Fails because the password is less than 5 characters.
Copy link
Contributor

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).

@cjyuan 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 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
Copy link
Contributor

@cjyuan cjyuan left a 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?

image

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.

Comment on lines 14 to 26
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;
}
Copy link
Contributor

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");
Copy link
Contributor

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
Copy link
Contributor

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.

Copy link
Contributor

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);
Copy link
Contributor

@cjyuan cjyuan Jul 4, 2025

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?

Copy link
Author

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.

Copy link
Contributor

@cjyuan cjyuan Jul 4, 2025

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.

Copy link
Author

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?

Copy link
Contributor

@cjyuan cjyuan Jul 4, 2025

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.

@cjyuan 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 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
return numericRank;
}
// Digits 2–9
if ("23456789".includes(rank)) return Number(rank);
Copy link
Contributor

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.

Copy link
Author

@Reza-Jahanimir Reza-Jahanimir Jul 5, 2025

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.

Copy link
Contributor

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
Copy link
Contributor

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?

Comment on lines 80 to 83
const banned = [];
isValidPassword("Valid1!", banned); // use it once
expect(isValidPassword("Valid1!", banned)).toBe(false); // second use, now rejected
});
Copy link
Contributor

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.

@cjyuan 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 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
Copy link
Contributor

@cjyuan cjyuan left a 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.

@cjyuan 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Complete Volunteer to add when work is complete and review comments have been addressed 📅 Sprint 3 Assigned during Sprint 3 of this module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants