How to handle a forEach loop that returns early based on an enum except for one path in Kotlin

user_626201 0
Viewed 35

We have an API service call that returns a bunch of validation messages. In each message there is a string that contains an error code.

Our implementation converts the validation string into an enum value and then we process the enumeration as there are some error code we just don't care about.

The question becomes, how to handle the loop of messages in a Kotlin way:

response.validationErrors?.forEach {
    val mediaFailure = decodeValidationMessage(it.message)

    if (mediaFailure != MediaFailure.Unknown) {
        return when (mediaFailure) {
            MediaFailure.Encrypted -> DomainResponse(ErrorReasonCode.ERR_DOCUMENT_ENCRYPTED)
            MediaFailure.NotSupported -> Response.validationFailed()
            MediaFailure.InternalError -> Response.serviceFailed()
            else -> throw NotImplementedError()
        }
    }
}

Here we loop through all the messages, then once the message error is not "Unknown" it returns the necessary response to the caller.

However, IntelliJ wants the else path, even though the if prevents that from happening.

Is there a proper Kotlin way of implementing this kind of loop?

kotlin
2 Answers

From what I understood, you want to return a response for the first mediaFailure which is not MediaFailure.Unknown and you don't want that throw NotImplementedError() part in your function. One way to fix this is to remove the if condition and continue the forEach loop when MediaFailure.Unknown is found.

response.validationErrors?.forEach {
    val mediaFailure = decodeValidationMessage(it.message)

    return when (mediaFailure) {
        MediaFailure.Encrypted -> DomainResponse(ErrorReasonCode.ERR_DOCUMENT_ENCRYPTED)
        MediaFailure.NotSupported -> Response.validationFailed()
        MediaFailure.InternalError -> Response.serviceFailed()
        MediaFailure.Unknown -> return@forEach // continue the loop
    }
}
user_133089910

I think this is one of the many cases when it pays to step back from the code a bit and try to look at the big picture. To ask “What's the ultimate goal here? What am I trying to achieve with this code?”

(In traditional, lower-level languages, almost anything you want to do with a list or array requires a loop, so you get into the habit of reaching for a for or while without thinking. But there are often alternative approaches in Kotlin that can be more concise, clearer, and harder to get wrong. They tend to be more about what you're trying to achieve, rather than how.)

In this case, it looks you want to find the first item which decodes to give a known type (i.e. not MediaFailure.Unknown), and return a value derived from that.

So here's an attempt to code that:

val message = response.validationErrors?.asSequence()
    ?.map{ decodeValidationMessage(it.message) }
    ?.firstOrNull{ it != MediaFailure.Unknown }

return when (message) {
    MediaFailure.Encrypted -> DomainResponse(ErrorReasonCode.ERR_DOCUMENT_ENCRYPTED)
    MediaFailure.NotSupported -> Response.validationFailed()
    MediaFailure.InternalError, null -> Response.serviceFailed()
    else -> throw NotImplementedError()
}

This is still fairly similar to your code, and it's about as efficient. (Thanks to the asSequence(), it doesn't decode any more messages than it needs to.) But the firstOrNull() makes clear what you're looking for; and it's obvious that you go on to process only that one message — a fact which is rather lost in the original version.

(If there are no valid messages, message will be null and so this will return serviceFailed(), as per comments.)

There are of course many ways to skin a cat, and I can think of several variations. (It's often a worthwhile exercise to come up with some — if nothing else, it gives you more confidence in the version you end up with!) Try to pick whichever seems clearest, simplest, and best matches the big picture of what you're doing; that tends to work out best in the long run.

user_101342090
Related