pakoito
commented
6 years ago Re. flatMap and the current API: even if the Scala code looks similar it doesn't do the same. Don't fall in that trap. It's rewritten behind the scenes the same way coroutines do to a chain of operations. The return type is a Try<List<T>>, so to be equivalent your example would have to return SuccessOrFailure<List<T>>, and the same has to be returned from the other methods.
So taking the original code:
fun getURLContent(url: String): List<String> {
val url = parseURL(url) // here parseURL returns URL, throws on failure
val connection = url.openConnection()
val input = connection.getInputStream()
val source = Source.fromInputStream(input)
return source.getLines()
}What happens when parseURL(url), openConnection(), getInputStream() return SuccessOrFailure<T>? Do you have to use the getOrNull and nullcheck it every line? Then you lose information about the exception, and makes it more verbose. If you have to getOrThrow and try/catch every line then you don't have any improvement over the current state of the world with normal exceptions. I'll write 4 snippets each with a different way of chaining these operations and are all valid (save for bugs :P) with the proposed API:
fun getURLContent(url: String): SuccessOrFailure<List<String>> {
val url = try {
parseURL(url).getOrThrow()
} catch (e: ParseException) {
return SuccessOrFailure.failure(e)
}
val connection = try {
url.openConnection().getOrThrow()
} catch (e: ConnectionException) {
return SuccessOrFailure.failure(e)
}
val input = connection.getInputStream().getOrThrow() // anything thrown here is non-recoverable
val source = try {
Source.fromInputStream(input).getOrThrow()
} catch (e: IOException) {
return SuccessOrFailure.failure(e)
}
return SuccesOrFailure.success(source.getLines())
}fun getURLContent(url: String): SuccessOrFailure<List<String>> {
val url = parseURL(url).getOrNull()
if (url != null) {
val connection = url.openConnection().getOrNull()
if (connection != null) {
val input = connection.getInputStream().getOrThrow()
val source = Source.fromInputStream(input)
if (source != null) {
return SuccessOrFailure.success(source.getLines())
} else {
return SuccessOrFailure.failure(SomethingException("1"))
}
} else {
return SuccessOrFailure.failure(ConnectionException())
}
} else {
return SuccessOrFailure.failure(ParseException())
}fun getURLContent(url: String): SuccessOrFailure<List<String>> {
return runCatching {
val url = parseURL(url).recover { if (it is ParseException) { return SuccessOrFailure.failure(it) } else { throw it } }.getOrThrow()
val connection = url.openConnection().recover { if (it is ConnectionException) { return SuccessOrFailure.failure(it) } else { throw it } }.getOrThrow()
val input = connection.getInputStream().getOrThrow()
val source = Source.fromInputStream(input).recover { if (it is IOException) { return SuccessOrFailure.failure(it) } else { throw it } }.getOrThrow()
source.getLines()
}
.recover { if (it is InputStreamException) { throw it } else { return SuccessOrFailure.failure(it) } }
}fun getURLContent(url: String): SuccessOrFailure<List<String>> {
try {
val url = parseURL(url).getOrThrow()
val connection = url.openConnection().getOrThrow()
val input = connection.getInputStream().getOrThrow()
val source = Source.fromInputStream(input).getOrThrow()
return SuccessOrFailure.success(source.getLines())
} catch (e: ConnectionException) {
return SuccessOrFailure.failure(e)
} catch (e: ParseException) {
return SuccessOrFailure.failure(e)
} catch (e: InputStreamException) {
throw e // so it doesn't get caught by IOException
} catch (e: IOException) {
return SuccessOrFailure.failure(e)
}
}None of the snippets is an improvement over the current situation of try/catching, and the API is ripe for abuse as it doesn't provide a clear, unique way of chaining SuccessOrFailure. The non-local returns required for coroutine rewrites also enable more possibilities for breaking control flow, and will create a copy of frequently called APIs on every callsite.
Every company, library, and employee will have their own preferred style, which means every time you come to a file using SuccessOrFailure it will look different and have different control flow that's not the one JetBrains had in mind for this proposal. You're trading learning a concept once, for all libraries, with having to read an implementation to understand its behavior every time.
If you let the exception propagate then your API is lying to you, it's not a SuccessOrFailure<List<T>>, it's a hidden exception instead like most Java APIs do. It's not an improvement, it's a step back with a new broken way of writing APIs.
You have the body of work that Arrow has done around this already. The flatMap way of chaining constructs is principled and shared under other names like bind, andThen and selectMany across JavaScript, C#, Java, TypeScript, Rust, Scala, Clojure, Haskell, F#, OCaml, ReasonML, a handful of libraries for each, and even other parts of Kotlin. It's well understood that flatMap chaining is not equivalent to exception propagation even with checked exceptions, nor the two snippets provided are the same and the alternatives described above are even more verbose than regular exceptions.
We have the implementation, the test suite, the documentation, entire communities of untyped languages that understand the concept and enough examples across all of them. I don't understand the pushback.
raulraja
elizarov
ktkit
fvasco
Wasabi375
Redrield
qwwdfsad
This issue is for discussion of the proposal to provide a Standard Library class to encapsulate successful or failed function execution.
Proposal text: result.md