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

Abstracted regex checker to a util function #1669

Closed
wants to merge 12 commits into from
Closed

Conversation

ifiok
Copy link
Contributor

@ifiok ifiok commented Jun 2, 2018

Issues

Closes:

Changes

The Regexp that validated the content type is used in two different places. Make it a const and share

@ifiok ifiok closed this Jun 2, 2018
function validateExtendedJsonContentType(contentType) {
var type = contentType.toLowerCase();
// map any +json to application/json
var jsonPatternMatcher = new RegExp('^application/[a-zA-Z.]+\\+json');

Choose a reason for hiding this comment

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

Interesting to see this RegExp being rebuilt on every call to the function. I built a little test that ran this function with the RegExp declared outside the function and found it to be about 460% faster. Is there a reason to leave it declared in the function rather than as a module var?

Copy link

@mattbishop mattbishop Jun 2, 2018

Choose a reason for hiding this comment

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

Here is the test script if interested:

console.time('RegExp outside');
var jsonPatternMatcher = new RegExp('^application/[a-zA-Z.]+\\+json');
for (var i = 0; i < 100000; i++ ) {
  jsonPatternMatcher.test('application/json');
}
console.timeEnd('RegExp outside');

console.time('RegExp inside');
for (var i = 0; i < 100000; i++ ) {
  var jsonPatternMatcher = new RegExp('^application/[a-zA-Z.]+\\+json');
  jsonPatternMatcher.test('application/json');
}
console.timeEnd('RegExp inside');

Copy link
Member

Choose a reason for hiding this comment

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

No reason not to! @ifiok seems like something went awry with your branch history - hope you don't mind, but I took a stab over in #1670

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