Can this code be shortened? ```fun getValue(value: Value, param: MutableMap<Char, Int>): Pair...

# codereview

[therealbluepandabear]

Can this code be shortened?

fun getValue(value: Value, param: MutableMap<Char, Int>): Pair<Char, Int> {
    return when (value) {
        Value.MIN -> Pair(param.keys.elementAt(param.values.indexOf(param.values.minOrNull() ?: 0)), param.values.minOrNull() ?: 0)
        Value.MAX -> Pair(param.keys.elementAt(param.values.indexOf(param.values.maxOrNull() ?: 0)), param.values.maxOrNull() ?: 0)
    }
}

Thanks, Tom

[aleksey.tomin]

fun getValue1(value: Value, param: MutableMap<Char, Int>) =
        when (value) {
            Value.MIN -> param.values.minOrNull() ?: 0
            Value.MAX -> param.values.maxOrNull() ?: 0
        }

    fun getValue2(value: int) = Pair(param.keys.elementAt(param.values.indexOf(value)), value)

    fun getValue(value: Value, param: MutableMap<Char, Int>) = getValue2(getValue1(value, param))

[ephemient]

fun getEntry(value: Value, param: Map<Char, Int>): Pair<Char, Int>? {
    val entry = when (value) {
        Value.MIN -> param.entries.minByOrNull { it.value }
        Value.MAX -> param.entries.maxByOrNull { it.value }
    } ?: param.entries.find { it.value == 0 }
    return entry?.toPair()
}

[ephemient]

the ?: fallback is of dubious utility. the only case when there is no min/max value is when the map is empty, in which case you won't find a 0 value either.

[ephemient]

I question the function, parameter, and type names as well, and if this is going to be a common operation you probably want a different data structure than a plain Map

[therealbluepandabear]

@ephemient what data structure would you use instead?

[ephemient]

depends on what your operations are

[therealbluepandabear]

@ephemient its just a simple application I am making... I am analysing some data; just for fun.

[ephemient]

at the very least, this single function alone suggests indexing the input by what is currently the map's values. so perhaps a SortedMap<Int, Set<Char>>, perhaps with an auxiliary Set<Char> to maintain the former keys' uniqueness

[ephemient]

if you need lookup in both directions, then build a bimap

[therealbluepandabear]

@ephemient ok. thanks for this information.

[ephemient]

at the very least, you can write this particular function without double linear traversal of all the entries

[therealbluepandabear]

@ephemient ok thanks

[elect]

fun getValue(value: Value, param: MutableMap<Char, Int>): Pair<Char, Int> {
    val value = when (value) {
        Value.MIN -> param.values.minOrNull()
        Value.MAX -> param.values.maxOrNull()
    } ?: 0
    return param.keys[param.values.indexOf(value)] to value 
}

[elect]

operator fun MutableMap<Char, Int>.get(value: Value): Pair<Char, Int> {
    val value = when (value) {
        Value.MIN -> values.minOrNull()
        Value.MAX -> values.maxOrNull()
    } ?: 0
    return keys[values.indexOf(value)] to value 
}

[ephemient]

indexOf() is still a second scan that you don't need. the key point is that entries.minByOrNull { it.value } entries.maxByOrNull { it.value } gives you both the key and value at once

[ephemient]

also MutableMap is unnecessary here

[therealbluepandabear]

@ephemient yes but I was using a mutable map in the code in 'MAIN' what can I use instead?

Cheers,
Tom

[ephemient]

@therealbluepandabear you can define it on Map<>, it will accept MutableMap<> as input