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

Regex test early on, remove redundant check #80

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

xoliver
Copy link

@xoliver xoliver commented Sep 20, 2023

Hi! I noticed that the regex already enforces that there should only be an @ between two lumps of text, so an early check with the regex will:

  • Save a redundant check
  • Speed up the process by saving the following checks

@houd1ni
Copy link
Collaborator

houd1ni commented Sep 29, 2023

Hi! Thank you for the idea! But is it really more performant to run the big regex than to create an array of two strings? Any tests ?

@xoliver
Copy link
Author

xoliver commented Sep 29, 2023

Hi @houd1ni . Thanks for your comment. This is my thinking:

  • For valid email addresses, all the checks need to happen. In that respect, my proposal spares an if so it should be the same performance or very marginally better
  • For invalid email addresses in cases that are specifically sought, such as missing "@" or components that are too long, the old way should be faster
  • For everything else (trailing spaces, invalid chars, etc), my suggestion will be faster as the regex will discard them upfront

I'm not a JS benchmarking expert (or JS experienced!) but I found this out and put out an example here, based on the input used in the unit tests: https://www.measurethat.net/Benchmarks/Show/27639/0/email-validator-pr-80-benchmark

My proposal seems to be 50% faster
image

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.

2 participants