Provide `mapIfInstance`?

[nikclayton]

Here's an extension function that might be useful to include.

/**
 * Maps this [Result<V, E>][Result] to [Result<V, E>][Result] by either applying the [transform]
 * function to the [value][Ok.value] if this [Result] is [Ok&lt;T>][Ok], or returning the result
 * unchanged.
 */
public inline infix fun <V, E, reified T : V> Result<V, E>.mapIfInstance(transform: (T) -> V): Result<V, E> {
    contract {
        callsInPlace(transform, InvocationKind.AT_MOST_ONCE)
    }

    return when (this) {
        is Ok -> (value as? T)?.let { Ok(transform(it)) } ?: this
        is Err -> this
    }
}

(that's using the 1.2x library version)

The motivating example is if you have a flow of Result where the OK value is one of a sealed set of subclasses and you typically only want to transform one of them. For example, Data.Loaded in this snippet:

sealed interface Data {
    data object Loading : Data
    data class Loaded(val data: String) : Data
}

sealed interface AppError {
    data class NetworkError(...)
    data class JsonError(...)
    // etc
}

Suppose you're processing a flow of Result<Data, AppError>

At the moment, with map, you have to write something like:

theFlow.map {
    when (it) {
        is Data.Loading -> it
        is Data.Loaded -> { /* some transform here */ }
    }
}

[Filtering the flow to just Ok(Data.Loaded) is not necessarily appropriate, as code "downstream" of the flow might still need to distinguish between these cases]

This rapidly gets tedious if you have to do this on every Flow<Result<V, E>>.

With this extension this becomes:

theFlow.mapIfInstance<_, _, Data.Loaded> { /* some transform here */ }

or

theFlow.mapIfInstance { it: Data.Loaded -> /* do something */ }

[michaelbull]

Thanks for the comprehensive writeup @nikclayton . I'm not really a fan of this proposal as it hides the instance checking logic, which is your app's business logic, inside the Result's libraries implementation.

If I was reading the example proposed, I would rather it read like:

fun main() {
    theFlow
        .map(::addMetadataIfLoaded)
       .andThen(::debugLog)
}

fun addMetadataIfLoaded(result: Result<Data, LoadingError>) {
    return when (result) {
        isOk && value is Data.Loaded -> /* add metadata here */
       else -> result
    }
}

This is much more opaque to the reader and produces (a) a nicer call-site chain, (b) a path to business logic that the reader can follow. Having this logic embedded deep in the Result library itself makes your own code less extensible, as you lose the ability to add other branches to your addMetadataIfLoaded function that you might want to handle in future (such as a new third state that represents something that 'loaded' but returned a picture instead of a video in the case of fetching some remote media file).

Furthermore, the current example only facilitates mapping instances of the happy-path. Adding the happy-path solution would then beg the question as to why we can't do it in the unhappy-path, at which point you now need better naming to distinguish it. We would then end up with something like mapIfOkAndIsInstance and mapIfErrAndIsInstance. This is quite a mouthful, and unfortunately without being this explicit it is not clear to the reader what value (the success or the error value) is actually being assigned to the instanceof check.

Ultimately, I would like the library and it's out-of-the-box functions to be dealing with Ok and Err, and not doing checks for you on the actual underlying value, as theoretically this list could extend to somthing ludicrous like checking that the int value of a Result<Int, Error> is greater than some number, like result.mapIfOkAndGreaterThan(300). That sort of business logic is better left as part of your app instead.