funkia / list

🐆 An immutable list with unmatched performance and a comprehensive functional API.
MIT License
1.65k stars 51 forks source link

multi-field sort #78

Closed Alx-l closed 4 years ago

Alx-l commented 5 years ago

Hi,

First, great lib 👍

I wanted to know if you were planning on adding a method allowing to sort on multiple fields? I played with your codebase, and I implemented (naïvely) it like this:

type SortOption<T> = {
  by: (item: T) => string | number
  reverse?: boolean
}

function sortByMultipleFields(opts: SortOption<T>[], l: List): List {
  if (l.length === 0) {
    return l;
  }
  let arr = l;

  arr = [].concat(l.prefix, l.suffix) // quick and dirty...

  arr.sort((a, b) => {
    for (let index = 0; index < opts.length; index++) {
      const { by, reverse = false } = opts[index];

      if (by(a) < by(b)) return reverse ? 1 : -1;
      if (by(a) > by(b)) return reverse ? -1 : 1;
    }

    return 0;
  });

  let newL = emptyPushable();
  for (let i = 0; i < arr.length; ++i) {
    push(arr[i], newL);
  }
  return newL;
}
exports.sortByMultipleFields = sortByMultipleFields;

Then, you can use it like this:

const items = L.list(
  { id: 2, name: 'bb' },
  { id: 1, name: 'bb' },
  { id: 2, name: 'aa' },
  { id: 1, name: 'aa' }
)

const sortedItems = items.sortByMultipleFields([
  { by: i => i.id }, // first I want the items sorted by their id
  { by: i => i.name, reverse: true }, // then, by their name, but in reverse order
])

// sortedItems = [
// { id: 1, name: 'bb },
// { id: 1, name: 'aa },
// { id: 2, name: 'bb },
// { id: 2, name: 'aa' },
// ]
paldepind commented 5 years ago

Hi @Alx-l. Why did you close this one? I don't have much time right now (I have exams). But I'd like to take a look at this later.

Alx-l commented 5 years ago

Hi @paldepind,

I closed it because I thought it slipped through the cracks 😀

Good to know thats it's not the case, let me know if you have some questions when you'll have the time to take a look at it.

paldepind commented 5 years ago

Hi again @Alx-l. I'd like to hear what you think about the following idea.

The function sortBy currently let's you return any value that is comparable using < (or the Fantasy Land Ord specification). What if it also handled any Iterable, like arrays or lists, and then compared them lexicographically. That is, by first comparing the first element, then the second, and so on.

That would make it possible to sort an object first by its id and then its name pretty easily with the following code:

const sortedItems = items.sortBy(i => [i.id, i.name]);

I like how this approach is just a generalization of sortBy. The biggest downside is that this idea lacks anything equivalent to the reverse flag that your proposal has. For numbers one can easily just negate them an achieve a reverse sort on them. But, for strings there is no easy way to do that.

What do you think? Is this idea good enough or do you think the reverse feature is important enough to warrant an additional function?

emmanueltouzery commented 5 years ago

regarding this, FYI prelude-ts supports:

vector.sortOn(elt=>elt.a,{desc:elt=>elt.b})

to say "sort by a, then by b descending".

paldepind commented 5 years ago

@emmanueltouzery

Making sortBy variadic is definitely also a nice way to extend it.

The change from ToOrderable<T> to { desc: ToOrderable<T> } looked a bit weird to me at first. But I can definitely see the appeal. It makes for a syntactically very compact way of declaring descending order.

Basically the following from the proposal

const sortedItems = items.sortByMultipleFields(
  [{ by: i => i.id }, { by: i => i.name, reverse: true }]
)

Shrinks down to

const sortedItems = items.sortOn(
  i => i.id, { desc: i => i.name }
)

Maybe it would make sense to do both.

emmanueltouzery commented 5 years ago

yeah the first version is more readable... prelude-ts also supports sortBy((a,b) => <return ordering>) btw.

if i would code it today i'd probably rather go for your first longer version to be honest. but now i have something and it works fine so if i don't get complaints i guess i won't touch it. I've had some thoughts about renaming sortOn and sortBy in prelude-ts for clarity, but never came up with good names too.

Alx-l commented 5 years ago

Hi @paldepind , @emmanueltouzery,

If the feature is already implemented in prelude-ts, then might as well use it :)

No need to reinvent the wheel in my opinion, I just thought it was rather useful to be able to sort on multiple fields and also be able to reverse order (by field).

But in that case, I think it would be great to make sortOn variadic then, to be able to sort on any number of fields, and not being limited to two.

Alx-l commented 4 years ago

Hello,

I'm going to re-close this issue ^^

If someone wants to sort by multiple fields, one simply could use function composition.

example:

import * as L from 'list/curried'
import pipe from 'lodash/fp/pipe'

type Item = {
  label: string
  id: number
}

const items: Item[] = [
  { label: 'two', id: 2},
  { label: 'second one', id: 1 },
  { label: 'three', id: 3 },
  { label: 'one', id: 1 },
  { label: 'four', id: 4 },
]

const listItems = L.from(items)

const sortedItems = pipe<L.List<Item>, L.List<Item>, L.List<Item>>(
  L.sortBy(i => i.label),
  L.sortBy(i => i.id)
)(listitems)

const result = L.toArray(sortedItems)
/* 
  result: 

  [ { label: 'one', id: 1 }, 
  { label: 'second one', id: 1 }, 
  { label: 'two', id: 2 }, 
  { label: 'three', id: 3 }, 
  { label: 'four', id: 4 } ]
*/