-
Notifications
You must be signed in to change notification settings - Fork 22
Add ABA routing number validation endpoint and tests #115
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: main
Are you sure you want to change the base?
Add ABA routing number validation endpoint and tests #115
Conversation
* isAbaRouting("021000021"); // true (Valid) | ||
* isAbaRouting("000000000"); // false (Invalid) | ||
*/ | ||
static isAbaRouting ( inputString ) |
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.
@mikeaig4real I noticed there was some differences in the core implementation details of the abaRouting function from the npm validator
library.
It looks like the regex string is missing some characters and digits? can you explain the differences? does this solution cover the same cases?
I believe it's fine to follow more closely the solution from that library. I personally find that the checksum code with if statements is more readable than the nested ternary expression
https://github.com/validatorjs/validator.js/blob/master/src/lib/isAbaRouting.js#L6
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.
Really appreciate the feedback @drewg2009!
You're right — I intentionally didn’t follow validator.js's implementation exactly. I wanted to take a slightly different approach by focusing on explicitly allowing valid prefixes (a whitelist) rather than blocking invalid ones (a blacklist). This made the regex easier to reason about from a "what's allowed" perspective.
Regarding the omission of the 80 prefix (used for traveler's checks), that was also intentional since it’s rarely relevant in modern banking — but I’m open to revisiting that if needed.
Our implementation currently passes all the same test cases as validator.js, and I actually included theirs as a sanity check too.
That said, I’m happy to align closer if we feel it would make maintenance or clarity easier long-term. Would you prefer we adopt their regex format or checksum style more closely? I'm open to either refining or leaving it as-is depending on what we value most.
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.
PS: Just to add — I’ve already updated our code to include the 80 prefix in the regex and switched the checksum logic to use the if/else style for clarity. Just need to commit and sync it based on your response to my previous message.
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.
As long as the code functions the same for the same inputs that is okay with me! I would still update the ternary to if blocks for easier readability, but other than that it's okay! I would also ensure that all edge cases are covered
…ikeaig4real/readableRegex into feature/add-isAbaRouting-route
@mikeaig4real approved! Only need to resolve conflicts and it's good to go, nice job! |
Introduce a new endpoint for validating ABA routing numbers, including comprehensive tests to ensure accuracy and reliability of the validation logic.