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

Refactor a List.generate to List.filled #87

Merged
merged 3 commits into from Jun 4, 2020
Merged

Refactor a List.generate to List.filled #87

merged 3 commits into from Jun 4, 2020

Conversation

natebosch
Copy link
Member

@natebosch natebosch commented Jun 3, 2020

The meaning is more clear without the indirection of a closure that
always returns the same value, and it will have better performance.

Also replace the List.insert by making the length longer to start
with and conditionally setting the first value.

Bump min SDK to allow for-loop elements in collection literals.
@natebosch
Copy link
Member Author

I was originally going to write this with a List.filled since the closure that always returns the same value is unnecessary and likely inefficient. Upon a closer look I think the list literal version is more clear than needing a list.insert(0 call to put a new element at the beginning of the list. In the list literal form the order and size of the list are both more clear in my opinion.

Copy link
Member

@kevmoo kevmoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Worth an entry in the changelog?

...seems like this might could improve perf

@jakemac53
Copy link
Contributor

...seems like this might could improve perf

Probably not, its possible but last I checked (recently) collection-for is still slower 🤷

@natebosch
Copy link
Member Author

collection-for is still slower

I could do

    separators = List.filled(newParts.length + 1, style.separator);
    if (!isAbsolute || newParts.isEmpty || !style.needsSeparator(root)) {
      separators[0] = '';
    }

WDYT?

@natebosch
Copy link
Member Author

To be clear, my primary goal is readability and I don't plan on spending time trying to microbenchmark this.

Now that I wrote out this new List.filled version I think I might even be leaning towards it as my preference for most readable...

@jakemac53
Copy link
Contributor

jakemac53 commented Jun 4, 2020

I am fine with either version, I wouldn't focus on micro-benchmarking this. I just wanted to refute the idea that collection for is fast, cause it isn't today :D.

@natebosch natebosch changed the title Refactor a List.generate to a list literal Refactor a List.generate to List.filled Jun 4, 2020
@natebosch
Copy link
Member Author

Worth an entry in the changelog?

No longer changing the min SDK so shouldn't be anything user visible. Likely does improve perf but I'm not going to bother benchmarking and I don't think it's worth calling out specifically in the changelog.

@natebosch natebosch merged commit 4f3bb71 into master Jun 4, 2020
@natebosch natebosch deleted the list-literal branch June 4, 2020 18:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants