refactors module for performance/readability

[le0nik]

Love the idea, just wanted to propose a refactor of the module. All tests pass successfully.

• Adds all used lodash functions to separate variables for readability purposes.
• Adds jsdoc for functions
• Saves links to objects into variables
• Replaces lodash.isArray to Array.isArray, also replaces lodash's type checks to native typeof for performance
• Removes unnecessary closure _putAsideIn, which is not needed and made us create variables outside of the middleware function's scope. Access to those variables was slower, than to those inside the scope, so there was no point in keeping it.
• Other minor improvements

[coveralls]

Coverage remained the same at 100.0% when pulling 33464ed on le0nik:refactor into c2be807 on analog-nico:master.

[analog-nico]

Hey @le0nik that's awesome! It seems you currently work on another commit to make the TravisCI build succeed. Just let me know when you are done!

I just reviewed your changes and I like them. One thing though: Everything that is executed inside hppMiddleware should be as performant as possible. Therefore please move _firstIfArray and _whitelistContainsKey outside of _fnPutAsideIn or at least outside of putAside so these functions are created only during startup and not for each request.

[analog-nico]

The input validation already ensures that whitelist is either null or an array. That means checking it here is unnecessary and reduces performance. Please revert it to if (whitelist) {.

[le0nik]

I'd disagree here. We can expect array or something else. So I think we should be explicit in what we expect. if (whitelist) checks for a truthy value. I think the more explicit we are the better it is for maintaining it in the future. And the performance gain in convertion Boolean(whitelist), that happens behind the scenes vs Array.isArray(whitelist) is negligible. What do you think?

[analog-nico]

Good point, I didn't think of the boolean conversion.

FYI, the reason I have a very strong opinion here is that this middleware eats CPU for each request. Considering that a product app has dozens of middlewares in use, inefficiencies all of them contain add up. However, I am just guessing how much CPU may or may not be wasted. Maybe we should write a performance test in the future.

The strength of your PR certainly is to improve readability and thus maintainability. Inspired by that how about this?

if (whitelist !== null) { // Validation above ensures whitelist is either null or an array.

AFAIK this would be the most performant version of this if.

[coveralls]

Coverage remained the same at 100.0% when pulling 32d4644 on le0nik:refactor into c2be807 on analog-nico:master.

[le0nik]

I see your point, that this module should be very performant, especially in the middleware function, so I refactored it a bit to improve performance in that function and maintain readability outside of it. What do you think? Suggestions?

P.S.: You weren't picky, btw. I agree, that it was the right thing to do. I just hope that the module itself isn't going to grow too much to loose the ability to maintain it's multiple loops.

[coveralls]

Coverage remained the same at 100.0% when pulling ede2dfc on le0nik:refactor into c2be807 on analog-nico:master.

[coveralls]

Coverage remained the same at 100.0% when pulling 3481d3c on le0nik:refactor into c2be807 on analog-nico:master.

[analog-nico]

Hi @le0nik thank you so much! You introduced some lines that may improve the performance even further. Just for fun I will work on this performance test on the weekend and also merge your changes then.

[le0nik]

Thanks a lot, @analog-nico! I want to also make a pull-request, that will allow for specifying whether the first or the last parameter should be used, if it's specified multiple times. I want it to stay first by default, like it is now, but give the user ability to choose the last one, because i think, that hardcoding first one to be used is very opinionated and there should at least be an option, that allows the user to change it. I'll make a pull-request if/when you merge this one in order not to introduce conflicts between the two. What do you think? Is it something you might consider merging?

[analog-nico]

Mmh, theoretically using the last value could be an option. However, if HPP kicked in we are in a situation that someone tried an attack. In this situation the first value is as good as the last value. HPP's job is only to make sure that the server always receives a single value - not an array that may bypass the input validation logic or produce errors.

Do you have a use case in your own project were choosing the first or the last value would make a difference?

[le0nik]

Can't say i have a use case at this point. I just think it's more intuitive to choose the last provided value. Most programmers(especially js programmers) are used to the paradigm, that whatever you set last overrides all the previously set values.

I'm sure, that the first value is as good as the last, but i suppose it won't hurt giving the developer and ability to choose which one to use with the default option set to 'first' for backwards compatibility. It's just a suggestion.

[analog-nico]

Indeed, devs expect the last to override the first! I would say, we just switch the behavior. Since it is an attack it is irrelevant which value is chosen. And thus we don't break the backwards-compatibility anyway. Providing an option would clutter the API in my opinion. Rhetorical question: Based on what should the dev choose the value of this option?!

If you agree I would just change it in the next release. Thoughts?

[le0nik]

Good point. I don't know of a reason the dev would choose one option or the another. And i agree on it not breaking backwards-compatibility. Can't imagine a case where dev would rely on it. I think i just felt it was counter-intuitive to choose the first one. The last one would be more predictable.

I'm all in for changing it in the next release. Want me to make a pull-request?

[coveralls]

Coverage remained the same at 100.0% when pulling 5a52cb1 on le0nik:refactor into c2be807 on analog-nico:master.

[analog-nico]

Hi @le0nik after implementing a performance test I compared the versions before and after your refactoring. By looking at the code I could have sworn the refactored code was faster. However, it was not. (0.006ms instead of 0.005ms per request) Interestingly, this change restored the original speed. One simple string concatenation. Unbelievable! shaking my head :D

[le0nik]

Hi, @analog-nico ! String concatenation is indeed very slow. I remember reading about it in High Performance Javascript by Nicholas C. Zakas. Didn't take it into account when i removed closure. Other than that the performance probably didn't change much, because V8 has very advanced JIT compilers, that make most micro-optimizations useless.

Anyway, congrats on implementing this benchmark!

Also, i think you could make the measurements even more accurate by replacing (new Date).getTime() with Date.now() or even hrtime(this wrapper provides a good abstraction for it: https://github.com/secrettriangle/present)

[analog-nico]

Thanks, "present" is a neat lib!

I just published version 0.2.0. Thanks again for your work!