NW | 25-ITP-Sep | TzeMing Ho | Sprint 3 | coursework/sprint-3-stretch #714

TzeMingHo · 2025-09-26T16:15:41Z

Learners, PR Template

Self checklist

I have titled my PR with Region | Cohort | FirstName LastName | Sprint | Assignment Title
My changes meet the requirements of the task
I have tested my changes
My changes follow the style guide

Changelist

In the stretch exercise, I have answered questions in find.js, created tests and a function for password validation, and created tests and a function for credit card.

illicitonion

This is looking really good - I left a few things to think about!

illicitonion · 2025-10-07T12:18:24Z

Sprint-3/3-stretch/creditCard-validator.js

+    return false;
+  }
+  // check if the sanitized input contains only digits
+  if (!/^\d{16}$/.test(sanitized)) {


By putting a {16} here, you're checking the length again. This means we're checking the length twice.

This is fine, but if we wanted to change the expected length of a credit card, we'd need to change both.

Can you think how to avoid this duplication?

Hi Daniel,
Thank you for pointing out that I have repeated the code. Regarding the issue, I have added a variable to store the value 16, and learned that if I want to apply the variable in a pattern, I need to call a new Regex function with the variable. It is something new and useful for me.

This works! There are a couple of other ways I could imagine going about this - can you think what they may be? (Happy to just discuss them in this comment thread, you don't need to change the code any more :))

I guess I understood what you meant by 'other ways.' Since we only have to check the length of 16 for once, the function will fail at the first condition check if the card number is not 16. I don't have to do it again in the second condition check. I could have focused on the characters that are all digits like /^\d+$/

Exactly :) Alternatively you could keep the {16} in the regex and delete the first length check completely (because any non-16-character string would fail the regex check).

illicitonion · 2025-10-07T12:22:29Z

Sprint-3/3-stretch/creditCard-validator.test.js

+describe("creditCardValidator", () => {
+  test("should return true for a 16 digit long card number", () => {
+    expect(creditCardValidator("1234-5678-9012-3456")).toBe(true);
+    expect(creditCardValidator("1234 5678 9012 3456")).toBe(true);


Your tests all currently assume there are extra characters in the credit card number - is 123456789012345 valid? You may want to test that kind of thing too, to make sure your implementation doesn't assume there are separators.

You are right. I should have included tests for card numbers that are more or fewer than 16 digits. As you suggested, I created one test for the case of fewer and another for more.

These extra tests look good, but aren't quite getting at the problem I was pointing at - can you add a test for a 16-digit credit card number which is only numbers (i.e. no spaces or -s)?

Yeah. I included a test for only 16 digits without spaces or operators.

Sprint-3/3-stretch/creditCard-validator.test.js

Sprint-3/3-stretch/find.js

Sprint-3/3-stretch/password-validator.js

Sprint-3/3-stretch/password-validator.test.js

illicitonion

This is looking great! I have yet more things to discuss, though, but hopefully they're all interesting!

illicitonion · 2025-10-08T10:56:05Z

Sprint-3/3-stretch/creditCard-validator.js

+  const sum = sanitized
+    .split("")
+    .reduce((total, digit) => total + Number(digit), 0);
+  if (sum <= VALID_LENGTH) {


It's not clear to me that the 16 here is related to the length of the number.

We want to be careful only to extract/share code where the reason for the sharing is the same. The first two places you use VALID_LENGTH are definitely both about the length, but here I would either put 16 back in, or make a separate constant MINIMUM_SUM or similar. Even though it happens to have the same value, if the reason is different, we don't want to share them. Otherwise e.g. if we changed the minimum sum, we may accidentally also change the expected length!

illicitonion · 2025-10-08T10:56:40Z

Sprint-3/3-stretch/creditCard-validator.js

+    return false;
+  }
+  // check if the sanitized input contains only digits
+  if (!/^\d{16}$/.test(sanitized)) {


This works! There are a couple of other ways I could imagine going about this - can you think what they may be? (Happy to just discuss them in this comment thread, you don't need to change the code any more :))

illicitonion · 2025-10-08T10:57:59Z

Sprint-3/3-stretch/creditCard-validator.test.js

+describe("creditCardValidator", () => {
+  test("should return true for a 16 digit long card number", () => {
+    expect(creditCardValidator("1234-5678-9012-3456")).toBe(true);
+    expect(creditCardValidator("1234 5678 9012 3456")).toBe(true);


These extra tests look good, but aren't quite getting at the problem I was pointing at - can you add a test for a 16-digit credit card number which is only numbers (i.e. no spaces or -s)?

illicitonion · 2025-10-08T11:06:44Z

Sprint-3/3-stretch/password-validator.js

+  const symbolCondition = /[!#$%.*&]/.test(password);
+  console.log(`symbol condition: ${symbolCondition}`);
+
+  const previousPasswords = [


This feels like it should probably be input to the function, rather than something we hard-code in the function itself?

Agree. I felt that it was something wrong to put a list of previous passwords inside a function. It should have been an input, which can be an array from a database or something very long and editable.

…iable name, and refactored previous password as a function input

TzeMingHo added 9 commits September 18, 2025 14:06

npm installed

9d43b8b

answer part a in find.js

b0265ba

answered part b to d in find.js

f1ac2fe

writing conditions and tests for password validation

78b6e92

added a break line in test file

1b11c5a

created credit card validator file and its tests file

596bf65

removing all space and - from the string input

bfae2f8

created tests and function for credit card validation

4eb2d5b

restore package-lock.json from origin/main

9c8e884

TzeMingHo added 📅 Sprint 3 Assigned during Sprint 3 of this module Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. labels Sep 26, 2025

illicitonion added Review in progress This review is currently being reviewed. This label will be replaced by "Reviewed" soon. and removed Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. labels Oct 7, 2025

illicitonion reviewed Oct 7, 2025

View reviewed changes

illicitonion added Reviewed Volunteer to add when completing a review with trainee action still to take. and removed Review in progress This review is currently being reviewed. This label will be replaced by "Reviewed" soon. labels Oct 7, 2025

TzeMingHo added 5 commits October 7, 2025 16:24

adding a variable for card valid length

f0d7b15

adding tests for card number length validation and only digit

d5d16fa

updating return statement in password-validator.js

5a7757f

updated password to validaPassword in tests

9447d08

adding tests for false cases

791b52a

TzeMingHo added Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. and removed Reviewed Volunteer to add when completing a review with trainee action still to take. labels Oct 7, 2025

illicitonion reviewed Oct 8, 2025

View reviewed changes

illicitonion added Reviewed Volunteer to add when completing a review with trainee action still to take. and removed Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. labels Oct 8, 2025

added a test for only 16 digits with spaces or operators, updated var…

70f9400

…iable name, and refactored previous password as a function input

TzeMingHo added Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. and removed Reviewed Volunteer to add when completing a review with trainee action still to take. labels Oct 9, 2025

Uh oh!

NW | 25-ITP-Sep | TzeMing Ho | Sprint 3 | coursework/sprint-3-stretch #714

Are you sure you want to change the base?

NW | 25-ITP-Sep | TzeMing Ho | Sprint 3 | coursework/sprint-3-stretch #714

Conversation

TzeMingHo commented Sep 26, 2025

Learners, PR Template

Changelist

Uh oh!

illicitonion left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

illicitonion left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!