LON | Amirhossein Aminian | Module-structuring-and-testing-data | sprint3#142
LON | Amirhossein Aminian | Module-structuring-and-testing-data | sprint3#142AmirhosseinAminian wants to merge 38 commits intoCodeYourFuture:mainfrom
Conversation
AmirhosseinAminian
added
the
Needs Review
SallyMcGrath
left a comment
SallyMcGrath
left a comment
There was a problem hiding this comment.
@Amir200524 , why have you submitted Java in .js files?
|
Hi @SallyMcGrath. I am so sorry about it. I converted them to JavaScript. Thank you. |
AmirhosseinAminian
added
the
Complete
Great, but can you talk a abit about what happened? |
|
Hi @SallyMcGrath I was watching some videos on YouTube about Java, and I probably answered some of them in Java. It’s so weird because I didn’t realize that VS Code was showing me some errors. |
cjyuan
left a comment
cjyuan
left a comment
There was a problem hiding this comment.
It doesn't seem anyone had comment on your code yet and there is a "Review Needed" label, so I am jumping in.
Code is great and seems to work correctly. I just have a few comments about the test cases and some suggestions.
| console.log(validateCreditCard("a92332119c011112")); // false (invalid characters) | ||
| console.log(validateCreditCard("4444444444444444")); // false (only one type of number) | ||
| console.log(validateCreditCard("1111111111111110")); // false (sum less than 16) | ||
| console.log(validateCreditCard("6666666666666661")); // false (odd final number) No newline at end of file |
There was a problem hiding this comment.
Why not check also cases where number of digits is not exactly 16?
| // If the character is a letter (either uppercase or lowercase), | ||
| // it rotates the character by the specified shift value within the alphabet, | ||
| // considering wrapping around if necessary. Non-letter characters are returned unchanged. | ||
| function rotateCharacter(char, shift) { |
There was a problem hiding this comment.
How would you modify your implementation if shift is allowed to be a negative number to represent a rotation in the opposite direction?
Sprint-3/implement/rotate-char.js
Outdated
| console.log(rotateCharacter("z", 1)); // Output: "a" (preserves case, but wraps around) | ||
| console.log(rotateCharacter("Y", 2)); // Output: "A" (preserves case, but wraps around) | ||
| console.log(rotateCharacter("z", 1)); // Output: "a" (wraps around) | ||
| console.log(rotateCharacter("Y", 2)); // Output: "A" (wraps around) |
There was a problem hiding this comment.
Why not test cases where shift is a large integer (e.g., 100, -100)?
| function isPrime(num) { | ||
| if (num <= 1) return false; | ||
|
|
||
| for (let i = 2; i <= Math.sqrt(num); i++) { |
There was a problem hiding this comment.
You can possibly improve the performance of the code in the following manners:
- Check if num is 2, and check only odd numbers >= 3 in the loop
- Avoid calling
Math.sqrt(num)repeatedly by assigning the value ofMath.sqrt(num)to a variable once, and then refer to the variable in the condition of the loop.- Note: The condition is checked at the start of every iteration.
| } | ||
|
|
||
| // Sample list of previous passwords | ||
| const previousPasswords = ["password123", "abc123", "welcome1"]; |
There was a problem hiding this comment.
- You can also export
previousPasswords. - The passwords in
previousPasswordsare all invalid passwords.
| }); | ||
|
|
||
| test('returns false for a valid password format but already used', () => { | ||
| expect(isValidPassword('welcome1', previousPasswords)).toBe(false); |
There was a problem hiding this comment.
The function could be returning false because "welcome1" does not have any uppercase letter. So this test may fail to check if the function can handle correctly cases where the password is already in used.
cjyuan
added
Reviewed
|
Hi @cjyuan thank you for your valuable tips. |
4 participants
Learners, PR Template
Self checklist
Changelist
Briefly explain your PR.
Questions
Ask any questions you have for your reviewer.