geefr / beatsaber-linux-goodies

Mod installation scripts and other goodies to support Beat Saber on Linux
BSD 2-Clause "Simplified" License
132 stars 5 forks source link

Sort list of beatsaber modifications before displaying them #77

Closed steffenWi closed 3 years ago

geefr commented 3 years ago

Okay, so sorting the mod list is sensible, but I can't accept these changes. Overall I'm intending to be picky in this codebase - main goals are to minimise breaking changes, and minimise maintenance needs (I'll have maybe one day a month for beatsaber).

So here's what breaks the rules

With that in mind I've done my version of this in #79 - @steffenWi Have I missed anything there? I think it's functionally the same but didn't spend much time on the C# docs.

steffenWi commented 3 years ago

Anything with the name helper, factory, manager, or similar are banned, very non-descriptive

understood. I do agree on 'factory' - mainly because that's a Java thing. But I, myself, prefer to have one class per file and instead have 'Helper' classes or 'Utility' classes or similar stuff so that I can use the same Comparer in different classes and namespaces without adding weird references. Just explaining my reasoning as to why I did it that way. No problem though :)

A comparator is right, but returning a list doesn't make sense if the list is always sorted (And there's no reason not to sort it)

From my understanding a SortedSet only has one advantage over a List and that is that when you add a new item to a SortedSet it will not be added to the end of the collection but instead at the index as determined by the sorting algorithm.

Meaning, if you have a SortedSet that contains the chars 'A' and 'C' at index 0 and 1 and it is alphabetically sorted, adding 'B' to it, would move 'C' to index 2 and insert 'B' at index 1. With a List, this wouldn't happen and 'B' would be at index 2.

Since this requires more operations/insert and in this project there is an ObservableCollection between the model and the View anyway and that there is no asynchronous stuff happening at the level the SortedSet vs. List thing happens, a List is, in my opinion, preferable.

Forcing the sort to en-US seems a bit odd, more complicated than it needs to be (If any locale was to be forced then it'd be en-GB, this is a British codebase ;D)

The code base may be British, though the Modlist seems to be en-US based, and that is what that is for. Admittedly, in case of sorting things I don't believe there would be a difference between any of the 'en' cultures. I suppose CultureInfo.InvariantCulture would've been the correct CultureInfo though.

geefr commented 3 years ago

Okay thanks for the input, an interesting take on things.

With things not being async there's certainly some performance issues in there, it's something I was going to improve at some point but had functionality/etc to get thrown together first.