morelinq / MoreLINQ

Extensions to LINQ to Objects
https://morelinq.github.io/
Apache License 2.0
3.63k stars 409 forks source link

refactored slice to avoid twice iteration #1011

Closed leandromoh closed 8 months ago

viceroypenguin commented 1 year ago

While there are two enumerable methods involved in the previous version, I don't see how it would have iterated the sequence twice - both Skip and Take are lazy, and neither will pull items unnecessarily. It would be good to update the tests for Slice to confirm this, though.

Furthermore, for non-list sequence, .Skip().Take() is already heavily optimized by the corelib. It turns into a single EnumerablePartition<TSource>() which handles both already. Take a look at the code here (Skip and Take are available in the same directory, and you can see that Skip creates the EnumerablePartition<> and Take calls EnumerablePartition<>.Take()). [note: since net6.0, Slice is actually an almost synonym for the bcl Take(IEnumerable<TSource> source, Range range) operator - Take(Range) is start..end and Slice(int, int) is start + count]

codecov[bot] commented 1 year ago

Codecov Report

Merging #1011 (d8fbc6b) into master (35fefdf) will decrease coverage by 0.01%. The diff coverage is 91.66%.

:exclamation: Current head d8fbc6b differs from pull request most recent head e221209. Consider uploading reports for the commit e221209 to get more accurate results

@@            Coverage Diff             @@
##           master    #1011      +/-   ##
==========================================
- Coverage   92.57%   92.57%   -0.01%     
==========================================
  Files         113      113              
  Lines        3422     3433      +11     
  Branches     1055     1060       +5     
==========================================
+ Hits         3168     3178      +10     
  Misses        191      191              
- Partials       63       64       +1     
Impacted Files Coverage Δ
MoreLinq/Slice.cs 92.00% <91.66%> (-0.86%) :arrow_down:

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

atifaziz commented 8 months ago

Closing this assuming it's unnecessary and abandoned.