#codereview
Title
# codereview
m

Mark

06/04/2022, 4:30 AM
Looking for feedback on naming and implementation. If all items transform to the same value, then that value is returned, otherwise null
Copy code
``````fun <T, R> Iterable<T>.singleOrNullBy(transform: (T) -> R): R? {
if (this is Collection && this.isEmpty()) return null
return asSequence()
.map(transform)
.zipWithNext()
.map { (a, b) -> if (a == b) a else null } // important we don't use mapNotNull here
.distinct()
.singleOrNull()
}``````
e

ephemient

06/04/2022, 5:38 AM
this can be inlined and returns early when possible:
Copy code
``````inline fun <T, R> Iterable<T>.singleOrNullBy(transform: (T) -> R): R? {
return fold(null) { acc: R?, item ->
val value = transform(item)
if (acc == null || acc == value) value else return null
}
}``````
although that doesn't quite handle
``null``
the same way; if you need that, a simple sentinel can do:
Copy code
``````inline fun <T, R> Iterable<T>.singleOrNullBy(transform: (T) -> R): R? {
val nothing = Any()
return fold(nothing) { acc: Any?, item ->
val value = transform(item)
if (acc === nothing || acc == value) value else return null
}.takeIf { it !== nothing } as R?
}``````
although as it returns the transformed value, not the original, I think it would be more consistently named
``singleOfOrNull``
m

Mark

06/04/2022, 6:57 AM
Thanks @ephemient I hadn’t thought of jumping out with a return like that. How about using an
``Iterator``
?
Copy code
``````inline fun <T, R> Iterable<T>.singleOfOrNull(transform: (T) -> R): R? {
// let's see if we can avoid creating the iterator
if (this is Collection) {
if (this.isEmpty()) return null
if (this is List && size == 1) {
return transform(this[0])
}
}
return this.iterator().singleOfOrNull(transform)
}

inline fun <T, R> Iterator<T>.singleOfOrNull(transform: (T) -> R): R? {
if (!this.hasNext()) {
return null
}
val firstItemTransformed = transform(this.next())
if (!this.hasNext()) {
return firstItemTransformed
}
this.forEach { item ->
if (transform(item) != firstItemTransformed) {
return null
}
}
return firstItemTransformed
}``````
But then again, might as well just do this (although perhaps not as performant):
Copy code
``````inline fun <T, R> Iterable<T>.singleOfOrNull(noinline transform: (T) -> R): R? {
// let's see if we can avoid creating the sequence
if (this is Collection) {
if (this.isEmpty()) return null
if (this is List && this.size == 1) {
return transform(this[0])
}
}
return this.asSequence().map(transform).distinct().singleOrNull()
}``````
Regarding naming, maybe
``singleDistinctOfOrNull``
because single by itself doesn’t say anything about distinct values.
e

ephemient

06/04/2022, 9:58 AM
if you're going to use a
``Sequence``
, just don't make the extension function
``inline``
to begin with, and see my other comment about being safe wrt. TOCTTOU and mutable collections. but sure, that's a reasonable name and straightforward implementation
note that you can also write
Copy code
``````inline fun <T, R> Iterable<T>.singleOfOrNull(transform: (T) -> R): R? {
val iterator = iterator()
if (!iterator.hasNext()) return null
val single = transform(iterator.next())
for (item in iterator) if (transform(item) != single) return null
return single
}``````
as Kotlin defines Iterator.iterator() to return itself
(by TOCTTOU issue, I mean that
Copy code
``````val list = CopyOnWriteArrayList(listOf(1))
list.singleOfOrNull { it }``````
could conceivably return
``2``
in your implementation, even though there is never a point in time where
``list == listOf(2)``
)
m

Mark

06/04/2022, 1:17 PM
Isn’t this the same as my
``Iterator``
-based implementation but without the quick checks? https://kotlinlang.slack.com/archives/C1H43FDRB/p1654337100738409?thread_ts=1654317012.586139&amp;cid=C1H43FDRB
e

ephemient

06/04/2022, 1:18 PM
1. changed
``.forEach``
to
``for``
2. your second quick check is vulnerable to TOCTTOU
m

Mark

06/04/2022, 1:27 PM
Copy code
``````inline fun <T, R> Iterable<T>.singleDistinctOfOrNull(transform: (T) -> R): R? {
// let's see if we can avoid creating the iterator
if (this is Collection && this.isEmpty()) {
return null
}
val iterator = this.iterator()
if (!iterator.hasNext()) {
return null
}
val firstItemTransformed = transform(iterator.next())
if (!iterator.hasNext()) {
return firstItemTransformed
}
for (item in iterator) if (transform(item) != firstItemTransformed) return null
return firstItemTransformed
}``````
e

ephemient

06/04/2022, 1:28 PM
sure, and you don't need the
``is List``
check either, but you aren't saving anything:
``.singleOrNull()``
uses
``iterator()``
itself
m

Mark

06/04/2022, 1:31 PM
Ah yes, very good points and well spotted regarding
``singleOrNull()``
using
``Iterator``
- I’ve just updated it.
2 Views