Any ideas to make this more compact or idiomatic?

# codereview

[Jukka Siivonen]

Any ideas to make this more compact or idiomatic?

[Jukka Siivonen]

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()

[Jukka Siivonen]

Maybe also without val results variable

[sbyrne]

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.

[christophsturm]

can it even have more than one exact match?

[Jukka Siivonen]

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)

[Jukka Siivonen]

But I can also accept suggestions for that case, where exact matches would be 0-1 🙂

[Tobias Suchalla]

As @sbyrne suggested:

If you only care for any first match:

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:

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.

[smichel17]

You can pull out a helper function, for something similar

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

[Jukka Siivonen]

👆 actually this is exactly what I did yesterday 🙂

[Jukka Siivonen]

And you're correct about "wrong abstraction vs. duplication" (seen too often avoiding of duplication at whatever the cost)