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 #2842

Merged
merged 4 commits into from Sep 22, 2021

Conversation

timotheeg
Copy link
Contributor

Context

This is a reopening of #2785 . That PR was reverted because it was using named capture groups (NCG) in regular expressions, and that is not supported in IE11.

The reopening of the PR removes the NCGs for compatibility. It makes the code slightly less readable, but it's good enough.

I was hoping to use babel and/or core-js to have auto-correction with compatibility of the NCGs in the regular expression (with @babel/plugin-transform-named-capturing-groups-regex) or core-js 3.18 (see this core-js PR that added support), but somehow I can't get either to run 😭 , and since this PR is not critical, I need to time-box and move on.

Old PR text is below.


Context (old)

While following code from the NIRC component, I ended up on the nric 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)

@timotheeg timotheeg force-pushed the refactoring/nirc_regex_improvements branch from 71e5727 to 228dcea Compare September 21, 2021 10:01
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

@timotheeg timotheeg merged commit 9d07224 into develop Sep 22, 2021
@timotheeg timotheeg deleted the refactoring/nirc_regex_improvements branch September 22, 2021 02:24
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.

None yet

2 participants