RemoveWarmupPeriods for Bollinger Bands needs ToList?

[martonb]

Just noticed, RemoveWarmupPeriods for Bollinger Bands uses a ToList, which looks unnecessary, to me... Any reason for it or just mistake?

[DaveSkender]

Sadly, yes, it's needed. In the current v2 code the utility method is this:

    public static IEnumerable<BollingerBandsResult> RemoveWarmupPeriods(
        this IEnumerable<BollingerBandsResult> results)
    {
        int removePeriods = results
            .ToList()
            .FindIndex(x => x.Width != null);

        return results.Remove(removePeriods);
    }

It has an IEnumerable input, but FindIndex only works on List types, hence the conversion. This is one of those tradeoffs with maintaining CLS compliance (the public API can't use a concrete List as an input).

I'll take a look at this for v3 to see if there's some better way to do this. In v3 we are working with IReadOnlyList and IList more, which you'd think would solve this issue but it doesn't, still needs the concrete List type. If you have another working idea, please do share.

• Tracking in RemoveWarmupPeriods has extra enumeration with ToList() #1314

[martonb]

I've only worked max .Net Framework 4.8, but I cloned the repo, looked at the code and noticed you are using LINQ functions (even if there are no using statements for it in the file, this confused me). Found their docs here, look at the SkipWhile functions, seems to do exactly what you need in a single step.
Additionally, I noticed in Pruning.cs' Remove function the same ToList() is used, you might be able to get rid of that too (if you need that function at all any more) by using Enumerable.Skip (described on same page I linked).
Sorry, I worked on tiny devices in C++, that's where this 'get rid of any extra operations'-mania comes from ;)
Just a heads-up: I also noticed some minor detections by SonarQube extension I use in Visual Studio... Nothing major, but might help you (it's a free static code analyzer).

Keep up the good work and a Happy New Year!

[DaveSkender]

I’m on the same page as you, and try to avoid extra memory copying like we’re doing here; and prefer to avoid LINQ overall in the library due to slower performance in general, especially for core code - I’ve been a bit lax on utilities. SkipWhile looks like a better approach.

Regarding static code analysis, we’re using Codacy, which may or may not have the same rules applied. If you see anything major, please do share.

[martonb]

Well, ToList() is a LINQ function, so your first argument of not using LINQ just burned :P
Also, AFAIK, it creates a new List instance with copy of all the elements from the source IEnumerable, which means:

• copy 1. is created just to search the index of the first non-null element, then it's thrown away
• copy 2. is created, from which elements are removed one by one, then this copy is actually returned

Assuming somebody, would write an automatic trading service, with multiple stocks, for each using multiple indicators, fed with new prices every second, this would badly trash the memory, as warmup periods on those lists are at times close to 100.
You have to agree, that would be bad...

I would just use LINQ or write a function that does get rid of null elements in a single step.
Also, I'd use List.RemoveRange, instead of removing one by one, where possible.

I never heard of Codacy, so thanks for the tip ;) I'll check it out.