London | 25-ITP-Sep | Payman Issa Baiglu | sprint 3 | 3-strech#885
Closed
PaymanIB wants to merge 1 commit intoCodeYourFuture:mainfrom
Closed
London | 25-ITP-Sep | Payman Issa Baiglu | sprint 3 | 3-strech#885PaymanIB wants to merge 1 commit intoCodeYourFuture:mainfrom
PaymanIB wants to merge 1 commit intoCodeYourFuture:mainfrom
Conversation
PaymanIB
added
the
Needs Review
cjyuan
reviewed
Nov 21, 2025
Contributor
cjyuan
left a comment
cjyuan
left a comment
There was a problem hiding this comment.
Code is working. I just have a few suggestions.
| const isLongEnough = password.length >= 5; | ||
| const hasUpper = /[A-Z]/.test(password); | ||
| const hasLower = /[a-z]/.test(password); | ||
| const hasNumber = /[0-9]/.test(password); |
Contributor
There was a problem hiding this comment.
Can consider name it hasDigit. '0', ..., '9' are called digits.
Comment on lines
18
to
-26
| test("password has at least 5 characters", () => { | ||
| // Arrange | ||
| const password = "12345"; | ||
| // Act | ||
| const password = "12345Aa!"; | ||
| const result = isValidPassword(password); | ||
| // Assert | ||
| expect(result).toEqual(true); | ||
| } | ||
| ); No newline at end of file |
Contributor
There was a problem hiding this comment.
Could also consider testing boundary cases:
- Valid password with 5 characters
- A 4-character password containing all required characters.
Comment on lines
+24
to
+46
| test("password missing uppercase letter", () => { | ||
| const password = "12345aa!"; | ||
| const result = isValidPassword(password); | ||
| expect(result).toEqual(false); | ||
| } | ||
| ); | ||
| test("password missing lowercase letter", () => { | ||
| const password = "12345AA!"; | ||
| const result = isValidPassword(password); | ||
| expect(result).toEqual(false); | ||
| } | ||
| ); | ||
| test("password missing number", () => { | ||
| const password = "Abcde!"; | ||
| const result = isValidPassword(password); | ||
| expect(result).toEqual(false); | ||
| } | ||
| ); | ||
| test("password missing special character", () => { | ||
| const password = "12345Aa"; | ||
| const result = isValidPassword(password); | ||
| expect(result).toEqual(false); | ||
| } |
Contributor
There was a problem hiding this comment.
It would be better to also specify in the test description
- the expected behavior of the function ("should return false when ..."), or
- indicate the test is for invalid passwords
cjyuan
added
Complete
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Password vaidator which is checking all the conditions required. I wrote it in 2 versins. 1st one is commented so the tests can pass.