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.

Sign up for GitHub

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
Open

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

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
41faaac
have done my key-implement exercise
Millena28 Feb 21, 2025
fb392b3
done the mandatory-rewrite
Millena28 Feb 21, 2025
52148a6
Done the mandatory-practice
Millena28 Feb 27, 2025
4d0497e
Done stretch investigate and implemented new card-validator file
Millena28 Feb 28, 2025
13220cd
done ALL
Millena28 Feb 28, 2025
396094b
made some corrections from review
Millena28 Mar 18, 2025
ccf0768
fixed the angle if statement, added validation steps
Reza-Jahanimir Jul 4, 2025
686efb6
solve the error in testing
Reza-Jahanimir Jul 4, 2025
435f1dd
solve function handling characters and validation.
Reza-Jahanimir Jul 4, 2025
cb18deb
now function differs between upper and lower characters. and instead …
Reza-Jahanimir Jul 4, 2025
c13ebd3
change the layout of my testing so that same test are grouped
Reza-Jahanimir Jul 4, 2025
37f8608
corrected the function
Reza-Jahanimir Jul 4, 2025
af01c38
omitted one the repeated statement , add validations
Reza-Jahanimir Jul 4, 2025
2b3e360
redesigning the function, specify the test
Reza-Jahanimir Jul 4, 2025
8dd332e
made the function look simpler and cleaner
Reza-Jahanimir Jul 4, 2025
e80b220
Numbers ending in 11, 12, 13 now are together in a test.
Reza-Jahanimir Jul 4, 2025
d32bc0c
i tried my best
Reza-Jahanimir Jul 4, 2025
ce330dd
function can return false when a password is one of the previous pass…
Reza-Jahanimir Jul 4, 2025
c969da7
add a line of explanation
Reza-Jahanimir Jul 5, 2025
95c85ba
change the function the its original, cleaned the test syntax
Reza-Jahanimir Jul 5, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 2 additions & 14 deletions Sprint-3/2-mandatory-rewrite/3-get-card-value.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,20 +10,8 @@ function getCardValue(card) {
if (rank === "A") return 11;
if (["K", "Q", "J", "10"].includes(rank)) return 10;

// It should be a string of digits, not starting with zero, and between 2-10
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;
}
// 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 •
edited
Loading

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


throw new Error("Invalid card rank");
}
Expand Down
12 changes: 9 additions & 3 deletions Sprint-3/3-mandatory-practice/implement/get-ordinal-number.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,15 +28,21 @@ test("append 'nd' to numbers ending in 2, except those ending in 12", () => {
test("append 'rd' to numbers ending in 3, except those ending in 13", () => {
expect(getOrdinalNumber(3)).toEqual("3rd");
expect(getOrdinalNumber(23)).toEqual("23rd");
expect(getOrdinalNumber(203)).toEqual("203rd");
expect(getOrdinalNumber(13)).toEqual("13th");
expect(getOrdinalNumber(203)).toEqual("203rd");
});

// all end in 'th'
test("append 'th' to all other numbers", () => {
expect(getOrdinalNumber(4)).toEqual("4th");
expect(getOrdinalNumber(5)).toEqual("5th");
expect(getOrdinalNumber(6)).toEqual("6th");
expect(getOrdinalNumber(10)).toEqual("10th");
expect(getOrdinalNumber(1000)).toEqual("1000th");
});

// unique cases 11, 12 ,13
test("handle unique cases correctly", () => {
expect(getOrdinalNumber(11)).toEqual("11th");
expect(getOrdinalNumber(12)).toEqual("12th");
expect(getOrdinalNumber(13)).toEqual("13th");
expect(getOrdinalNumber(1000)).toEqual("1000th");
});
20 changes: 19 additions & 1 deletion Sprint-3/4-stretch-investigate/password-validator.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ test("password has a unknown character", () => {
})


test("password has at least 5 characters", () => {
test("password has more 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?

expect(isValidPassword("1@Qe")).toBe(false); // Fails because the password is less than 5 characters.
});
Expand Down Expand Up @@ -64,6 +64,24 @@ test("password should not be a previously used password", () => {
expect(isValidPassword("Valid1!")).toBe(true);
Copy link
Contributor

@cjyuan cjyuan Jul 4, 2025 •
edited
Loading

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 •
edited
Loading

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 •
edited
Loading

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.

});

// Shouldn't we be testing if the function can return false when a password is one of the previous passwords?
test("should not accept a previously used password", () => {
const banned = ["Valid1!"];
expect(isValidPassword("Valid1!", banned)).toBe(false); // Should return false
});

// we care tring manually to add a password that has been used before
test("valid password", () => {
const banned = [];
expect(isValidPassword("Valid1!", banned)).toBe(true); // first time, valid
});

test("password should not be a previously used password", () => {
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.




// test("valid password", () => {
Expand Down