search

NCronJob-Dev / NCronJob

A Job Scheduler sitting on top of IHostedService in dotnet.
https://docs.ncronjob.dev/
MIT License
158 stars 11 forks source link

refactor: Simplify RemoveChars #82

Closed linkdotnet closed 5 months ago

linkdotnet commented 5 months ago

Small refactoring

linkdotnet commented 5 months ago

I took the liberty to enable C# 13 features for NCronJob. Wanted to get your thoughts on this one @falvarez1

linkdotnet commented 5 months ago

The new params collections are pretty nice. Since you made the method public how does that impact the ability to use NCronJob from a .Net 8, C#12 application? Is it binary/source compatible? If this can be used from a .Net 8 app without needing to support C#13 then keep it as-is, otherwise it'll need the corresponding preprocessor directives.

The class is still internal. I try to stick to Microsofts coding guidelines, where they make classes internal and every function public instead of internal as it seems redundant.

The params Span is a SDK thing. So you need netsdk 9 to compile this, but there is no problem in using in .NET 8. Otherwise our test suite hopefully wouldn't run!? Well at least that is my way of thinking, but we can easily skip that as this is a microoptimization.

falvarez1 commented 5 months ago

FYI, was curious about the difference in allocations so I ran a benchmark, seems to be negligible with the previous approach. image

linkdotnet commented 5 months ago

Interesting - really neglecable. While I am still in favor of Contains, I am more indifferent about params ReadOnlySpan<char>.