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

Replace for-loop with recursion #315

Closed
wants to merge 1 commit into from
Closed

Replace for-loop with recursion #315

wants to merge 1 commit into from

Conversation

mapogolions
Copy link

@mapogolions mapogolions commented Oct 11, 2019

I think it's pretty weird to use construction like this and all of those mutable local variables to keep track a current state. I propose to use recursion. The last seems more natural. I know that JS has some problems with tail rec optimization, but i I cannot imagine the case when a path will contain many dot-separated properties

@belochub
Copy link
Member

Hi, @mapogolions and thanks for the contribution.
Unfortunately, I don't think that these particular functions require such substantial changes at the moment.

IIRC this code was written in the current way to run the fastest, possibly sacrificing readability, because it was used a lot in hot paths. AFAIK recursion runs notably slower than loops at the moment, so the implementation you propose will most likely hurt the performance.

I agree with the fact that these functions can be written in a more clear way and I don't mind refactoring these functions without making the switch to recursion, so feel free to open another PR with the changes.

@belochub belochub closed this Dec 30, 2019
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.

None yet

2 participants