Skip to content
This repository has been archived by the owner on Jun 11, 2024. It is now read-only.

Unify indexOf & includes usage in exception handling - Closes #2436 #2581

Merged
merged 1 commit into from
Nov 28, 2018

Conversation

yatki
Copy link
Contributor

@yatki yatki commented Nov 26, 2018

What was the problem?

Exception handling has been spread throughout the application and not handled from a central place. Also includes and indexOf changes were being used inconsistently.

How did I fix it?

After discussing with @nazarhussain, I decided that we can't combine all exception handling into a central logic due to complexity of the current architecture. I only replaced inconsistent indexOf usages and used includes in all checks.

How to test it?

Review checklist

@yatki yatki self-assigned this Nov 26, 2018
@4miners
Copy link
Contributor

4miners commented Nov 26, 2018

Why indexOf over includes?

@yatki
Copy link
Contributor Author

yatki commented Nov 27, 2018

@4miners I checked the performance difference between indexOf and includes. Seems like indexOf is more performant on chrome and since we are using node I thought we should stick with indexOf.

See: https://stackoverflow.com/questions/47659972/array-indexof-vs-includes-perfomance-depeding-on-browser-and-needle-position

@yatki yatki force-pushed the 2436-optimize-handling-exceptions branch from 7893944 to e738a40 Compare November 27, 2018 13:03
@yatki
Copy link
Contributor Author

yatki commented Nov 27, 2018

@4miners after further investigation, I found out with latest version of V8 engine includes is almost as fast as indexOf. So I'm switching to use includes.

@MaciejBaj MaciejBaj merged commit f3c82dc into development Nov 28, 2018
@MaciejBaj MaciejBaj deleted the 2436-optimize-handling-exceptions branch November 28, 2018 16:39
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unify indexOf & includes usage in exception handling
3 participants