https://kotlinlang.org logo
#codereview
Title
# codereview
j

Jukka Siivonen

09/07/2021, 1:36 PM
Any ideas to make this more compact or idiomatic?
Copy code
val results = <get collection>
return results
            .filter { it.matchType == MatchType.EXACT }
            .also { 
                // Do logging
            }
            .singleOrNull()
            ?: results
                .filter { it.matchType == MatchType.PARTIAL }
                .also { 
                    // Do logging
                }
                .singleOrNull()
Maybe also without val results variable
s

sbyrne

09/07/2021, 1:46 PM
You could group by MatchType, then sort for Exact before Partial, then mapNotNull the singleOrNull, then firstOrNull. It would look more idiomatic, but I think would be less clear and less performant than what you did.
c

christophsturm

09/07/2021, 2:35 PM
can it even have more than one exact match?
j

Jukka Siivonen

09/07/2021, 5:50 PM
Valid question, yes it can (I prefer not to restrict it elsewhere although it would be possible, and handle here like in my initial solution)
But I can also accept suggestions for that case, where exact matches would be 0-1 🙂
t

Tobias Suchalla

09/08/2021, 9:27 AM
As @sbyrne suggested: If you only care for any first match:
Copy code
val results = getCollection().groupBy { it.matchType }
val matches = results[MatchType.EXACT] ?: results[MatchType.PARTIAL]
// log matches
return matches?.firstOrNull()
If you want exactly one
EXACT
match or else exactly one
PARTIAL
match:
Copy code
val results = getCollection().groupBy { it.matchType }
val match = results[MatchType.EXACT]?.singleOrNull()
        ?: results[MatchType.PARTIAL]?.singleOrNull()
// log match
return match
I'd say that both do look clearer than you original code. Can't comment on performance though.
👍 1
s

smichel17

09/09/2021, 1:39 PM
You can pull out a helper function, for something similar
Copy code
kt
val results = <get collection>
return results.singleOfType(MatchType.EXACT)
    ?: results.singleOfType(MatchType.PARTIAL)

private fun <T: HasMatchType>Collection<T>.singleOfType(type: MatchType): T? {
    return this
        .filter { it.matchType == type }
        .also {
            // Do logging
        }
        .singleOrNull()
}
However, a word of warning: only do this if you're certain that these parts aren't incidental duplication. It's easy to turn a code base into spaghetti trying to remove de-duplication when actually you're coupling pieces of functionality that are just similar, not the same. Which becomes a giant pain when you need to change one of them later and you need to un-tangle them, first :P
j

Jukka Siivonen

09/09/2021, 5:39 PM
👆 actually this is exactly what I did yesterday 🙂
And you're correct about "wrong abstraction vs. duplication" (seen too often avoiding of duplication at whatever the cost)
2 Views