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

Tidy up #82

Closed
wants to merge 7 commits into from
Closed

Tidy up #82

wants to merge 7 commits into from

Conversation

petecraven91
Copy link

Refactored the code so it isn't all in one file. broke out functions and the card types

Copy link
Contributor

@crookedneighbor crookedneighbor left a comment

Choose a reason for hiding this comment

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

Overall, I think it's a good move to split this large file into several smaller files, but I have a bunch of nitpicks I think should be addressed.

In addition, if we're pulling many of these things into units, now is probably the time to unit test those functions rather than treating them as private functions of the main module. If you do not want to do that work, we'll do that before merging in.

.eslintrc Outdated Show resolved Hide resolved
validator/cardTypes.json Outdated Show resolved Hide resolved
yarn.lock Outdated Show resolved Hide resolved
validator/helpers/findType.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated
@@ -1,296 +1,44 @@
'use strict';

var types = require('./validator/cardTypes.json');
var clone = require('./validator/helpers/clone');
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think it's necessary to have all these nested inside a helpers directory.

@petecraven91
Copy link
Author

@crookedneighbor all valid points. I've made the suggested changes
I agree this is the ideal time to add testing for each function, just don't have the time to do it right now but can look at doing this for future pull requests

Copy link
Contributor

@crookedneighbor crookedneighbor left a comment

Choose a reason for hiding this comment

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

Changes look great.

I just noticed that you have the file names in camel case. Braintree's normal style is to have file names in kebab case (lib/card-types.js). Can you make that change?

@petecraven91
Copy link
Author

Thanks
Updated to kebab case

Copy link
Contributor

@jackellenberger jackellenberger left a comment

Choose a reason for hiding this comment

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

Looks good to me! @crookedneighbor do you have thoughts vis a vis declaring a function then module.exports = function vs module.exports = function () { }?

@crookedneighbor
Copy link
Contributor

I don't care that much.

Usually, if it's a small function, all I'll just do: module.exports = function () {} and if it's longer, I'll name it.

@crookedneighbor
Copy link
Contributor

Will work on adding some additional tests for this on Friday.

@crookedneighbor
Copy link
Contributor

Didn't get a chance to work on this last week like I'd hoped. Will try to get this merged in this week.

@crookedneighbor
Copy link
Contributor

Merged in with 32c92e9

Made a few choices to combine some files. For instance, the matchesPattern and matchesRange functions could be considered private methods of the matches module, so I moved them in there and made sure to test both cases in the test file for matches.

Thanks for doing this chore!

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

3 participants