MoreLINQ icon indicating copy to clipboard operation
MoreLINQ copied to clipboard

SkipLast is not optimal

Open Orace opened this issue 6 years ago • 2 comments

~SkipLast use CountDown and therefor store all the elements.~ ~We can manage to store only the number of element to skip.~ ~Store the full sequence could and should be avoided.~

Orace avatar Oct 31 '19 18:10 Orace

Anyway the current implementation is not really optimal (intermediate object creation, test for null, etc...) For performance reason, I think that the MoreLinq methods should not call other MoreLinq methods but go straightforward to the most efficient implementation.

This is a big bite and a long running task. Anyway this is a start for it.

Orace avatar Oct 31 '19 20:10 Orace

For performance reason, I think that the MoreLinq methods should not call other MoreLinq methods but go straightforward to the most efficient implementation.

Yes, I agree with that. In general, the approach and steps to adding something non-trivial and useful has been:

  1. If it can be implemented in terms of other operations then do that first.
  2. Add unit tests.
  3. Get it working.
  4. Release for field use.
  5. Optimize in a future version (unit tests from 2 keeping us real at this stage), even if it means hand-rolling the implementation.

The benefit of 1 is that we can leverage all the tested operations.

We need to get coverage reporting added to the project so we can make sure tests are sufficiently covering the code before refactoring wildly. Meanwhile it's better to err on the safe side.

Also keep in mind that a lot of the operators in MoreLINQ can be implemented for asynchronous streams and that will explode the implementation base. When you reuse other operators, there will be far less implementation duplication. In the end, we have to strike a balance.

atifaziz avatar Nov 01 '19 08:11 atifaziz