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(isCreditCard): create allCards dynamically and get rid of hard-to-maintain hardcoded version #2117

Merged

Conversation

pano9000
Copy link
Contributor

@pano9000 pano9000 commented Dec 6, 2022

Hello,

I've refactored the way the allCards variable is created/handled in isCreditCard:
It currently is manually hardcoding all of the previously already defined RegExp into one huge RegExp, that it later then checks against.

This is problematic, because:

  • it adds unnecessary code duplication (you need to store the provider RegExps in two places: 1x on its own and 1x inside allCards
  • (at least to my eyes) it is unreadable
  • therefore it makes it a nightmare to update RegExps or add new providers

I've replace it with a dynamically created array with the RegExps from the cards object instead, which will make it a lot easier to maintain and update, when new providers are added:

  • you only need to add the RegExp inside the cards object
  • no manual copying of the RegExp necessary

Since we are now working with an array instead of one huge RegExp, I had to also change the last "else if":
There I am using the Array some() method, which checks if any of the items inside the array return true for a given function.

Checklist

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

get rid of the hardcoded allCards variable, which was a manual copy of the existing regExp for card provider.
Replace it with a dynamically created array instead, which will make it easier to maintain, when new providers are added.
@codecov
Copy link

codecov bot commented Dec 6, 2022

Codecov Report

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

Coverage data is based on head (1ecdd21) compared to base (531dc7f).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff            @@
##            master     #2117   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          104       104           
  Lines         2308      2313    +5     
  Branches       578       579    +1     
=========================================
+ Hits          2308      2313    +5     
Impacted Files Coverage Δ
src/lib/isCreditCard.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.

add "istanbull ignore else", similarly to how it is was done in:
9ee09a7
const tmpCardsArray = [];
for (const cardProvider in cards) {
// istanbul ignore else
if (cards.hasOwnProperty(cardProvider)) {
Copy link
Member

Choose a reason for hiding this comment

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

Why is this if-statement needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I originally did not have it in there either, but there is an linting rule that forced me to add it :-)
It otherwise threw me guard-for-in error
https://eslint.org/docs/latest/rules/guard-for-in

@brianwhaley
Copy link
Contributor

Interesting that we are doing this... i would have to look back, but i think that i had a similar solution to this in the first refactor I did. Glad it is coming back, makes maintenance much easier. Any card provider can be added very easily now.

@pano9000 pano9000 requested review from profnandaa and removed request for profnandaa March 10, 2023 00:07
Copy link
Member

@profnandaa profnandaa left a comment

Choose a reason for hiding this comment

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

LGTM, thanks.

@profnandaa profnandaa merged commit 4c25f26 into validatorjs:master Jun 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants