search

github / hotkey

Trigger an action on an element with a keyboard shortcut.
https://github.github.com/hotkey/
MIT License
3.22k stars 97 forks source link

Use indexOf to check several types #36

Closed Link2Twenty closed 4 years ago

Link2Twenty commented 4 years ago

Use indexOf to check several types at once rather than checking one by one

keithamus commented 4 years ago

Hey @Link2Twenty thanks for the issue!

What would be the justification for merging this? I'm not sure it offers readability improvements, and without benchmarks to measure we do not know if it faster (my hunch is that it isn't).

Link2Twenty commented 4 years ago

I believe indexOf is marginally faster but it's only about 1~2k ops/s

I tend to find it more readable this way but maybe that's from MySQL past and using IN all the time 😅

keithamus commented 4 years ago

I believe indexOf is marginally faster but it's only about 1~2k ops/s

I put together a small benchmark and I'm afraid the results don't match up to this expectation. The current code is roughly 1.5 billion ops/s faster.

I think the current code is adequate, I don't think this change is well justified given the benchmarks, and so I'm going to close this. If you feel differently please feel free to make the case and supply empirical evidence that this is a well justified change and we can re-open and merge.