-
Notifications
You must be signed in to change notification settings - Fork 1
fix(utils): correct bank number validation #917
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
base: master
Are you sure you want to change the base?
Conversation
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.
Please write unit tests for this method.
c1fe6b0
to
059b72c
Compare
.reduce((sum, digit) => sum + digit, 0) % | ||
10 === | ||
0; |
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.
This looks so ugly.
.reverse() | ||
.map((char, index) => { | ||
const digit = Number(char); | ||
return index % 2 === 1 |
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.
Prefer writing declarative code; use isEven
or isOdd
functions.
if (isNaN(Number(bankNumber))) { | ||
return FieldError.InvalidBankAccountNumber; | ||
} | ||
|
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 length of bank numbers varies between 9 digits to 12. We may use that as well before commencing on the luhn algorithm.
? digit * 2 > 9 | ||
? digit * 2 - 9 | ||
: digit * 2 | ||
: digit; |
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.
This is unreadable at all. It's logic is correct 100%, but it's hard to read. Try using if statements instead.
Moreover, try to write the algorithm yourself first:
Algorithm isValidBankAccount:
Input: bankNumber - string of the bank number
Output: true or false
sum <- 0
for each digit in the bank number do the following
d <- the digit
if the digit is an even number
sum <- sum + d
continue
...
Remember, you will never learn by allotting everything to the AI.
You way manna write the function in an empty JS file in order to be able to experiment and execute it easily with node.
import { | ||
generateValidLuhnNumber, | ||
generateInvalidLuhnNumber, | ||
} from "./luhnGenerator"; |
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.
Relative imports are not recommended.
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.
This's not how we write unit tests. Use AI as a tool only. You are the coder; read other unit tests files and follow its pattern or so to speak our implicit convention of writing tests.
the unit tests should be added in the
invoice.test.ts
file.
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.
Thank you for the note🫡
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.
First of all, only test files, like {entity}.test.ts
, are stored in the tests directory. This shouldn't be store here. You may write these functions within the file that uses them. Second, I don't believe this is the best approach. I prefer using just multiple valid and invalid bank account numbers.
We may use the ones being used by check50
:
https://github.com/cs50/problems/blob/2025/x/credit/__init__.py
Quality Checklist
Links
Important
Incase you have any valuable knowledge that you think that it is worth writing down, feel free to add it to our handbook. Wounder why we are doing this? check this guide.