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

Faster, more readable, less nested and more consistent function #1454

Merged
merged 1 commit into from
Nov 15, 2020
Merged

Faster, more readable, less nested and more consistent function #1454

merged 1 commit into from
Nov 15, 2020

Conversation

siddharthborderwala
Copy link
Contributor

@profnandaa I have updated the assertString utility function in src/lib/util/assertString.js.
The new function is more readable and has less nesting. It has less if/else nesting and also has a consistent error message (previously some error messages have "... received a [invalidType]" and some of them have "... received [invalidType]", now it is always "... received a [invalidType]").
I measured performances of the current function and the new function, it's a small margin but the new function is faster by 0.006 to 0.01 milliseconds on an average.
image
image
image

@codecov
Copy link

codecov bot commented Oct 4, 2020

Codecov Report

Merging #1454 into master will decrease coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1454      +/-   ##
==========================================
- Coverage   99.92%   99.92%   -0.01%     
==========================================
  Files          95       95              
  Lines        1268     1265       -3     
==========================================
- Hits         1267     1264       -3     
  Misses          1        1              
Impacted Files Coverage Δ
src/lib/util/assertString.js 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 685c3d2...950bdfc. Read the comment docs.

@profnandaa
Copy link
Member

Thanks for your contribution! 🎉 /cc. @tux-tn @ezkemboi

@siddharthborderwala
Copy link
Contributor Author

@profnandaa It was my pleasure to contribute to a library that is so vastly used! 😍

@profnandaa
Copy link
Member

@siddharthborderwala -- see if you can address the code coverage drop issue...

@siddharthborderwala
Copy link
Contributor Author

@profnandaa I generated the report locally and checked it, turns out the coverage issue is because of Line 89 in src/lib/isTaxID.js, apparently it is not getting hit according to the coverage report.
image
This is the report data for the directory in which I have changed files.
Screenshot 2020-10-05 183743

@profnandaa
Copy link
Member

I see, thanks! I think we will have this addressed in an open related PR.

@siddharthborderwala
Copy link
Contributor Author

Sounds okay. After updating this code, I ran npm run test and it caused a bunch of changes in other files because of the build steps I guess, should I commit and push those changes too? @profnandaa

@siddharthborderwala
Copy link
Contributor Author

@profnandaa Could you please label this pull request as hacktoberfest-accepted , I would appreciate that a lot, thanks!

@profnandaa
Copy link
Member

@siddharthborderwala -- thanks. Sure, the whole repo is opted into hacktoberfest, so this should count after landing even without the tag; let me know if it works.

@profnandaa
Copy link
Member

/cc. @tux-tn @ezkemboi @rubiin

@profnandaa profnandaa merged commit a19ebff into validatorjs:master Nov 15, 2020
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.

5 participants