London | ITP-Jan-26 | Mohsen Zamani | Sprint 3 | stretch#939
London | ITP-Jan-26 | Mohsen Zamani | Sprint 3 | stretch#939mohsenzamanist wants to merge 4 commits intoCodeYourFuture:mainfrom
Conversation
This comment has been minimized.
This comment has been minimized.
Sprint-3/4-stretch/card-validator.js
Outdated
| !cardNumArray.every( | ||
| (num) => num.charCodeAt(0) >= 48 && num.charCodeAt(0) <= 57 | ||
| ) |
There was a problem hiding this comment.
Note: We could also check if num is a digit as num >= '0' && num <= '9'.
Sprint-3/4-stretch/card-validator.js
Outdated
| const count = cardNumArray.reduce((acc, curr) => { | ||
| acc[curr] = acc[curr] ? acc[curr] + 1 : 1; | ||
| return acc; | ||
| }, {}); | ||
| if (Object.keys(count).length < 2) return false; |
There was a problem hiding this comment.
This works.
Challenge: Implement a simpler and more efficient way to check distinct digit count with Set.
| @@ -1,6 +1,12 @@ | |||
| const previousPasswords = ["5B43n21"]; | |||
| function passwordValidator(password) { | |||
There was a problem hiding this comment.
If we design the function as
function passwordValidator(password, previousPasswords=[]) {
...
}
we could let the caller specify what the previous passwords are.
| @@ -1,6 +1,12 @@ | |||
| const previousPasswords = ["5B43n21"]; | |||
There was a problem hiding this comment.
An invalid password is not a good candidate for a previous password.
| } | ||
| ); No newline at end of file | ||
| // Arrange | ||
| const password = "A1b2"; |
There was a problem hiding this comment.
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 "A1b2" not only because it is fewer than 5 characters, but also because it lacks a special 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.
| test("password contains at least one uppercase English letter", () => { | ||
| // Arrange | ||
| const password = "1a2345"; | ||
| // Act | ||
| const result = isValidPassword(password); | ||
| // Assert | ||
| expect(result).toEqual(false); | ||
| }); | ||
|
|
||
| test("password contains at least one uppercase English letter", () => { | ||
| // Arrange | ||
| const password = "1B2345"; | ||
| // Act | ||
| const result = isValidPassword(password); | ||
| // Assert | ||
| expect(result).toEqual(false); | ||
| }); |
There was a problem hiding this comment.
- These two tests have the same description but they are testing two different things.
There was a problem hiding this comment.
Could you make the test descriptions more detailed and descriptive? That way, if a test fails, the person implementing the function can quickly understand what went wrong and why.
Right now, for example, "password contains at least one number (0-9)" does not quite indicate what the test is testing. (Did you mean to say "not containing")
cjyuan
left a comment
There was a problem hiding this comment.
- Did you run Jest test after you made the changes? It seems one of the tests in
password-validator.test.jshas a bug and will fail.
Sprint-3/4-stretch/card-validator.js
Outdated
| if (cardNumArray.length !== 16) return false; | ||
|
|
||
| // checks if all digits are numbers | ||
| if (!cardNumArray.every((num) => num >= 0 && num <= 9)) return false; |
There was a problem hiding this comment.
if (!cardNumArray.every((num) => num >= 0 && num <= 9)) return false;
if (!cardNumArray.every((num) => num >= '0' && num <= '9')) return false;Both produce the same result, the latter expression is more a commonly used.
Performance wise, while it is hardly noticeable, the former expression is less efficient
because it requires the number to be converted to string first before the comparison takes place.
| expect(result).toEqual(false); | ||
| }); | ||
|
|
||
| test("should return false if password has no numbers(0-9)", () => { |
There was a problem hiding this comment.
Could shorten the description by using the term digit.
|
I ran jest and password-validator.test.js works perfectly, all the tests passed. |
cjyuan
left a comment
There was a problem hiding this comment.
Great work!
You have all the info I intend to pass to you. So I will go ahead and mark this PR as complete first.
| // Arrange | ||
| const password = "5B43n21!"; | ||
| // Act | ||
| const result = isValidPassword(password, "5B43n21!"); |
There was a problem hiding this comment.
I expected this test to fail because the second parameter isn't an array of strings.
I didn't realise string also has a method .includes() and your function manages to pass this test.
However, other implementations that expect the second parameter to be an array may fail this test unexpectedly.
There was a problem hiding this comment.
Appreciate all the comments.
I missed that it should be an array and it's surprising the code is working.
But if function explicitly checks for an array then it'll fail, so I'll add the check to the function and change the test as well.
Thanks.
Learners, PR Template
Self checklist
Changelist
Comelete exercises in directory called 4-stretch
Questions