https://kotlinlang.org logo
#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))
thread { list.add(2); list.add(3); list.remove(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