Skip to content
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

refactor: use modern JS constructs to validate NRIC #2785

Merged
merged 3 commits into from
Sep 20, 2021

Conversation

timotheeg
Copy link
Contributor

@timotheeg timotheeg commented Sep 13, 2021

Context

While following code from the NIRC component, I ended up on the nirc validation where I thought several minor improvements could be made. Below are some (very minor) issues I thought about when seeing the code

  • The regex is using unnecessary constructs like {1}
  • Regexp specifies both lowercase and uppercase matches, while it would be simpler to uppercase the input prior to running the test
  • The regexp captures groups, but these are not used because the format validation and checksum validation are disjointed
  • The digits string is exploded to iterate over the characters, but that is unnecessary since the coefficients is an array of the same format that could be used instead
  • We can us the string array operator to read digits from the input, and to read checksum markers (i.e. there's no need to create local arrays.)
  • the reduce() function does an unnecessary assignment to a local variable, which looks confusing because of the shadow variable name sum.

The code was working perfectly fine, so the refactor is unnecessary. I initially only wanted to remove the{1} and add a i flag to the regex, but I ended up refactoring the whole file 😑

I hesitated to open the PR since it is unplanned and unnecessary, and will consume precious reviewer time, but in the end, since I did the changes, I thought I'd open it up anyway for some discussions, like:

  • does using for named groups in regexes affect our browser support matrix?
  • does using array operator for string character access affect our browser support matrix? (do we have precedent in code?)

Approach

I've made changes that addresses all the points above. The code reduces significantly. I am obviously really biased, but I think it is more readable now.

Testing

  • Create a form with a NIRC component
  • Verify validation works as expected in major browsers (Chrome, FF, safari, Edge)

Copy link
Contributor

@mantariksh mantariksh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a few typo/casing suggestions first, will review the logic soon!

shared/utils/nric-validation.ts Outdated Show resolved Hide resolved
shared/utils/nric-validation.ts Outdated Show resolved Hide resolved
shared/utils/nric-validation.ts Outdated Show resolved Hide resolved
shared/utils/nric-validation.ts Outdated Show resolved Hide resolved
shared/utils/nric-validation.ts Outdated Show resolved Hide resolved
shared/utils/nric-validation.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@tshuli tshuli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm!

shared/utils/nric-validation.ts Outdated Show resolved Hide resolved
@timotheeg
Copy link
Contributor Author

timotheeg commented Sep 14, 2021

On my 2 questions from the PR description:

does using for named groups in regexes affect our browser support matrix?

Support matrix for named character groups
image

does using array operator for string character access affect our browser support matrix? (do we have precedent in code?)

String array-like character accessed was introduced in EcmaScript 5 (see doc here).

which has this support matrix:
image

I guess the biggest question I have now is whether we support IE11?

I just checked the transpiled (tsc+babel) result to check if by chance the code would be transformed to use older constructs like .charAt() to be more backward compatible, but it doesn't look like it does:

function(e,t,n){"use strict";Object.defineProperty(t,"__esModule",{value:!0}),t.isNricValid=void 0;var r=/^(?<prefix>[STFG])(?<digits>\d{7})(?<checksum>[A-Z])$/;t.isNricValid=function(e){var t=null==e?void 0:e.toUpperCase().match(r);if(!t)return!1;var n=t.groups,i=n.prefix,o=n.digits;return n.checksum===("S"===i||"T"===i?"JZIHGFEDCBA":"XWUTRQPNMLK")[[2,7,6,5,4,3,2].reduce((function(e,t,n){return e+t*parseInt(o[n])}),"S"===i||"F"===i?0:4)%11]}}

I will defer merging this for now, and maybe schedule a short discussion for our next sync.

@timotheeg
Copy link
Contributor Author

Confirmed with Sonjia we do not support IE11, so I'm just going to merge this now.

@timotheeg timotheeg merged commit aaa8072 into develop Sep 20, 2021
@timotheeg timotheeg deleted the refactoring/nirc_regex_improvements branch September 20, 2021 01:16
@timotheeg timotheeg restored the refactoring/nirc_regex_improvements branch September 21, 2021 06:28
timotheeg added a commit that referenced this pull request Sep 21, 2021
timotheeg added a commit that referenced this pull request Sep 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants