Closed j2ko closed 7 years ago
This looks good to me, but I think @azertyfun should take a look as well, since it's his original piece of code.
Nice work! I would sugggest deleting insert()
and remove()
from the code, then I can merge it.
Done
I am probably missing something, but is there a reason we are not doing the sorting in the SQL query?
I guess we will switch to SQL based ordering eventually.
We cannot sort (or do any kind of meaningful operation) on a collection's app list in SQL, as it is stored as a JSON. SQLite doesn't support lists/arrays, so short of creating a table for each collection, this is our only option AFAIK.
apps.tag_ids
, apps.supported_platforms
, apps.screenshots
and apps.versions
suffer from the same problem unfortunately.
From what I see (and again, maybe I am looking at it wrong), we currently get a list of IDs (which is in JSON), but then for each ID, we query for its associated app. Could we instead concatenate the IDs in to an "IN" query?
Something like (pseudocode):
idList := strings.Join(appIds, ", ")
handler.Query("SELECT id, name, type ...etc FROM apps WHERE id IN ? ORDER BY published_date", idList
Not that any of this is truly necessary, just may be a good optimization down the road.
Oooh yes that is clever indeed. I'm going to go ahead and do some performance testing, because it might very well be worth it as it currently takes several hundred milliseconds to generate a large collection.
This pull request replaces custom sorting approach by standard library sort implementation. Now apps could be sorted by using Comparator predicate.
NOTE: it use
sort.Slice
that is part of Go 1.8