Added isAbaRouting validator

[songyuew]

Added isAbaRouting validator

Check whether a string is ABA routing number for US bank account & cheque

https://en.wikipedia.org/wiki/ABA_routing_transit_number

Please review my PR, thanks!

Checklist

• PR contains only changes related; no stray files, etc.
• README updated (where applicable)
• Tests written (where applicable)

[codecov]

Codecov Report

Base: 100.00% // Head: 100.00% // No change to project coverage 👍

Coverage data is based on head (4bf84a7) compared to base (43803c0).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff            @@
##            master     #2143   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          106       107    +1     
  Lines         2348      2361   +13     
  Branches       593       597    +4     
=========================================
+ Hits          2348      2361   +13     

Impacted Files	Coverage Δ
src/index.js	100.00% <100.00%> (ø)
src/lib/isAbaRouting.js	100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[pano9000]

If I am not mistaken I don't think it is necessary to split the string's characters into an array ->
you can access each character in the string via its index directly as well.
I've also tested it locally here and it works with the tests you provided.

[pano9000]

According to the ABA document [1] (page 4), there seems to be quite a few ranges, that are "reserved for future use", i.e. they are currently not valid ABA numbers, at least that is the way I would see it.

I think it would make sense to adjust the RegExp accordingly to reflect that, what do you think?
If you agree, don't forget to also update the tests, as I can see at least one potentially "invalid" test case that is currently in the "valid" section (789456124 would fall under these "reserved for future use" number ranges)

[1] https://s3-eu-west-1.amazonaws.com/cjp-rbi-accuity/wp-content/uploads/2016/09/21222732/ROUTING_NUMBER_POLICY-2016.pdf

[songyuew]

Hi Panogiotis,

You are right about this. I have just double-checked their latest article. I added a separate regex to filter out reserved routing numbers, so that it can be easily modified if reserved series is brought into assigned in the future.

Thanks for your review!

[pano9000]

Hi Songyue,

thanks for the updates.
One thing I probably would still change is the RegExp -> instead of checking the Aba number with two separate RegExp, you could combine that into one single RegExp -> according to some quick benchmarks I did here locally, this executes 2x faster.

We can make make this happen with one regexp only, by using a "negative lookahead" like so:

/^(?!(1[3-9])|(20)|(3[3-9])|(4[0-9])|(5[0-9])|(60)|(7[3-9])|(8[1-9])|(9[0-2])|(9[3-9]))[0-9]{9}$/

I do agree, that this is a tiny bit harder to read though, but this seems to be the fastest way to get this done :-)
What do you think?

If you agree, would you update the RegExp accordingly please?
Thank you!

---

Out of curiosity (and also a tad bit of boredom ;-)) I also did try out two other ways to achieve the same thing, while keeping it a bit easier to read.
Both attempts work as well and are arguably easier to read, but they do seem to be quite a lot slower than just using that RegExp (in my benchmark they were ~10x slower this way)

Attempt 1: Use an Array for the reservedRanged and join them into a new RegExp
Slower than RegExp, but alot easier to read

  const reservedRanges = [
    '1[3-9]',
    '20',
    '3[3-9]',
    '4[0-9]',
    '5[0-9]',
    '60',
    '7[3-9]',
    '8[1-9]',
    '9[0-2]',
    '9[3-9]',
  ]

  const isRoutingReg = new RegExp( `^(?!(${reservedRanges.join(')|(')}))[0-9]{9}$`)

Attempt 2: Use String Concatenation,
slower than Regexp only, a bit faster than joining an array, but a bit less readable

  const reservedRanges = 
    '(1[3-9])|' +
    '(20)|' +
    '(3[3-9])|' +
    '(4[0-9])|' +
    '(5[0-9])|' +
    '(60)|' +
    '(7[3-9])|' +
    '(8[1-9])|' +
    '(9[0-2])|' +
    '(9[3-9])'

  const isRoutingReg = new RegExp( `^(?!${reservedRanges})[0-9]{9}$`)

Benchmark tests:

test	result
messy single line	1,109,643 ops/sec ±0.38% (97 runs sampled)
two separate regexp	554,690 ops/sec ±0.73% (87 runs sampled)
string concat	116,022 ops/sec ±0.58% (92 runs sampled)
array join	82,380 ops/sec ±0.42% (92 runs sampled)

Fastest is messy single line

[pano9000]

thank you, looks good to me now :-)

(however please note: I am not a maintainer, so my approval is not really as helpful for getting this merged - we will need to wait for the maintainers to approve and pull - and unfortunuately this might take some time)

---

one tiny small side note, just to avoid any further questions:
the Regexp theoretically can still be tightened, e.g. that last part (9[0-2])|(9[3-9]) could be combined into a single (9[0-9]), however I would say it makes sense to keep this "uncombined" as it is now, as this mimics the "official" ranges from the ABA document

[songyuew]

Hi Panagiotis,

Yes, I have the same opinion, as they are defined as two series by ABA.

Thank you again for your proposals! They are all sound and practical.

Just for my curiosity, for the RegEx benchmarking, which tool did you use? It is very useful to get number of ops per sec.

[pano9000]

Just for my curiosity, for the RegEx benchmarking, which tool did you use? It is very useful to get number of ops per sec.

I am using Benchmark.js for these, as it is pretty straight forward to set up those comparisons.

However from what I read online, we of course should always be "cautious" with such benchmark tools, as they may lead to "inaccurate" results or wrong assumptions sometimes, depending on the test case.
So always take the results with a grain of salt :-)

[pano9000]

Sorry about not noticing before, but I would say that str.trim() probably does not belong here?
In my opinion trimming the string falls under the "sanitation step" and should be kept out of this "validation step".

Otherwise you might end up with a scenario as below:

• User inputs <space>teststring<space>
• validator trims the string from
• validator runs its validation on the now internally sanitized input and returns e.g. true
• user thinks that their string is valid, when in reality the validator actually validated the sanitized string
• user ends up continuing working with the untrimmed (theoretically) invalid string

Does that make sense?
Let me know, what you think.

[pano9000]

Looks good to me now, that the sanitization is removed :-)
thanks

[profnandaa]

LGTM. Please fix the m/c and we'll be good to go.

[songyuew]

LGTM. Please fix the m/c and we'll be good to go.

Thank you @profnandaa , M/C now fixed.

[profnandaa]

BTW, this is just for US only? Do we plan to support other countries and should we have a locale param then?
Sorry missed this in my previous review.

[songyuew]

Yes, ABA is the abbv for American Bankers Association. The ABA number is used with every US bank account and cheque, to identify the corresponding financial institution. For example:

There is no need to add the locale param.

[profnandaa]

Does any other country have a similar thing perhaps just with a different name? Trying to see if there's a way we could generalize here than having a whole validator just for one country...

[songyuew]

Great idea. I'll look into it. 😁