Add optional sortBy function

[JanGorman]

Why

Currently the sorting of elements returned from the Store is done adhoc. The Demo code does this inside the view layer.

For the use case I have in mind, the sorting logic is a bit more complex and the same store is used in several places throughout the app. Duplicating the sorting logic in several places like this doesn't seem like a good idea.

How

The PR adds an optional sortBy: (Item, Item) -> Bool function to the Store.

[mergesort]

Hey @JanGorman! Really appreciate not only the idea, but the initiative to put up a pull request. I'll have to take a closer look at this over the next few days, I'm currently pretty heads down on some work. Hope that's ok, I want to make sure I give this the same thought and consideration that you gave the patch!

[JanGorman]

Thanks @mergesort. I totally understand, take your time!

[mergesort]

Sorry this took me a bit @JanGorman, between releasing the first public beta of my app and taking the time to think through this change it took me longer to get around to this than I'd hoped. That did give me some time to figure out what I wasn't sure about the approach posted here, and how I'd consider correcting it.

The reason I've considered adding a sortBy parameter before and ended up deciding not to is because this is code that isn't inherently meaningful to put in the Store. I would be more inclined to add it if there was some way that it tapped into the underlying StorageEngine, for example if it made the underlying SQLiteStorageEngine's SQL query more efficient.

The purpose of sorting is almost always for display purposes, and a Store can be accessed in multiple places, both of those interactions happen the app level rather than at the library layer. I understand that's why you made the parameter optional, so users can opt out, but I feel a little hesitant about moving that logic down into the Store rather than something like an extension on Array at the app level. An example of where this is a concern is that if we decide we no longer want to sort but instead want to filter, we would have to add a filterBy parameter to the initializer as well.

For the use case you describe I would likely express it as such, centralizing the logic in one place without having to repeat your sorting code.

public extension Array where Element == Tag {

    mutating func sort(by: (Item, Item) -> Bool) {
        ...
    }

    mutating func filter(where: (Item, Item) -> Bool) {
        ...
    }

    mutating func someComplexLogicYouCanCentralizeAndCallOnTheStoresItems(…) {
        ...
    }

}

I'd like to know what you think of that, and I always aim to be constructive so I'd also be happy to think through another potential option I've considered before. It may be worth exposing a function that lets you do arbitrary but centralized changes to the underlying array, whether that's filtering, sorting, or any other modification. I'm not 100% sure how it should look but I've considered something with the shape of (name tbd) process(_ items: inout [ Item ]). I still think that should likely live at the app-layer, but I'd be happy to have a very open-minded discussion.

How does all of that sound Jan?

[JanGorman]

Thanks for taking the time and the very thorough answer 🙏

And you mention exactly the two points that crossed my mind as well:

• that once you start adding the option to sort, where do you draw the line or do you end up adding parameters for all methods that exist on Array…
• how could this be made more efficient by leveraging SQL

I think the alternative that you propose to wrap access to the Store in a centralised place in the app is a valid approach that I'll explore further.

Again thanks for the answer and for putting out the library. Appreciated!

[mergesort]

Thanks a lot for the effort and understanding @JanGorman, both are genuinely appreciated! 🙇🏻‍♂️