-
-
Notifications
You must be signed in to change notification settings - Fork 76
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
Karla Grajales | Sprint-3 - Module-Structuring-and-Testing-Data | SPRINT 3 #238
base: main
Are you sure you want to change the base?
Conversation
…ype.test.js and we can run the test
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 code are pretty solid.
I left you some comments and suggestions.
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.
Changes look good. I just have few suggestions. Feel free to mark this as Completed.
const rank = input.slice(0, -1); | ||
|
||
// Check if the rank is a valid numeric value or face card | ||
if (!isNaN(rank) && Number(rank) >= 2 && Number(rank) <= 10) { |
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.
A string representing a decimal numbers such as "2.3" could still pass this test.
A bulletproof approach would be to check if rank
is exactly one of "2", "3", ..., "10" before doing the number conversion.
} | ||
|
||
// If numerator is equal to denominator, it's not a proper fraction | ||
if (numerator === denominator) { |
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 condition at line 49 can also handle this case.
|
||
// Calculate the new character code | ||
// const rotatedCode = (upperLetter.charCodeAt(0) - baseCode + num) % 26 + baseCode; | ||
const rotatedCode = (upperLetter.charCodeAt(0) - baseCode + num + 26) % 26 + baseCode; |
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.
If we allow num
to be a very "large" negative number (e.g., -1000), we can rewrite line 30 as
const rotatedCode = (upperLetter.charCodeAt(0) - baseCode + num % 26 + 26) % 26 + baseCode;
Note: num % 26
would reduce any value of num
to the range [-25, 25].
}); | ||
|
||
test("password too short", () => { | ||
expect(passwordValidation("Shor!", existingPasswords)).toBe(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.
"Shor!" has 5 characters. :)
thank you @cjyuan I will have a look this again, thank you so much for your time. |
Learners, PR Template
Self checklist
Changelist
In this project, I learned to create simpler, well-structured functions without overcomplicating them. I focused on interpreting instructions carefully to ensure the code behaves as expected. Additionally, I explored how to install Jest, write test cases, and debug them effectively. I also learned to use assertions to validate functionality and handle errors by throwing them instead of returning them, which helped improve the clarity and reliability of the code.
Questions
Ask any questions you have for your reviewer.