refactor(isCreditCard): create allCards dynamically and get rid of hard-to-maintain hardcoded version

[pano9000]

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)

[codecov]

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.

[WikiRik]

Why is this if-statement needed?

[pano9000]

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]

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.

[profnandaa]

LGTM, thanks.

[pano9000]

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)

[codecov]

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.

[WikiRik]

Why is this if-statement needed?

[pano9000]

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]

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.

[profnandaa]

LGTM, thanks.

[pano9000]

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)

[codecov]

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.

[WikiRik]

Why is this if-statement needed?

[pano9000]

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]

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.

[profnandaa]

LGTM, thanks.

[pano9000]

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)

[codecov]

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.

[WikiRik]

Why is this if-statement needed?

[pano9000]

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]

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.

[profnandaa]

LGTM, thanks.

[pano9000]

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)

[codecov]

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.

[WikiRik]

Why is this if-statement needed?

[pano9000]

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]

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.

[profnandaa]

LGTM, thanks.

[pano9000]

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)

[codecov]

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.

[WikiRik]

Why is this if-statement needed?

[pano9000]

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]

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.

[profnandaa]

LGTM, thanks.

[pano9000]

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)

[codecov]

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.

[WikiRik]

Why is this if-statement needed?

[pano9000]

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]

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.

[profnandaa]

LGTM, thanks.

[pano9000]

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)

[codecov]

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.

[WikiRik]

Why is this if-statement needed?

[pano9000]

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]

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.

[profnandaa]

LGTM, thanks.