Optimize distinctBy implementation for non-empty collections #4711
Conversation
| } | ||
|
|
||
| NonEmptyList(head, buf.toList) | ||
| NonEmptyList.fromListUnsafe(bldr.result()) |
There was a problem hiding this comment.
I think it would be slightly better to do:
var rest = tail
seen.add(f(head))
while (rest.nonEmpty) {
val next = rest.head
rest = rest.tail
if (seen.add(f(next)) {
bldr += next
}
}
NonEmptyList(head, bldr.result())the main changes:
- we don't have to build an iterator for the list, we just directly iterate on List
- we don't have to allocate the outermost List just to destructure it inside NonEmptyList.fromListUnsafe.
There was a problem hiding this comment.
I'd also suggest to check if the source NeList has 1 item only (tail.isEmpty or use sizeCompare) and simply return this in that case.
It would help to avoid allocating unnecessary structures: the builder and tree set and recreating 1-item NeList.
This small optimization could also be applied to the other two collections.
| } | ||
|
|
||
| NonEmptySeq(head, buf.result()) | ||
| NonEmptySeq.fromSeqUnsafe(bldr.result()) |
There was a problem hiding this comment.
this one is different from NonEmptyList, because it is a wrapper on the whole Seq, unlike NonEmptyList which is a class.
Given that, this implementation here seems good to me.
|
Thanks, I’ve implemented your comments. Hope it's good now. |
| @@ -345,16 +345,22 @@ final case class NonEmptyList[+A](head: A, tail: List[A]) extends NonEmptyCollec | |||
| override def distinctBy[B](f: A => B)(implicit O: Order[B]): NonEmptyList[A] = { | |||
| implicit val ord: Ordering[B] = O.toOrdering | |||
There was a problem hiding this comment.
actually this could be moved inside the else branch to avoid it on singletons.
Addresses #4610