android / architecture-samples

A collection of samples to discuss and showcase different architectural tools and patterns for Android apps.
Apache License 2.0
44.3k stars 11.62k forks source link

Replace `Result` with exception handling #906

Closed dturner closed 1 year ago

dturner commented 1 year ago

Here's what I've done:

johnjohndoe commented 1 year ago

Can you please explain why you made the changes?

dturner commented 1 year ago

Can you please explain why you made the changes?

Great question, sure.

Result is used to indicate success or failure when reading data. In the "success" case, the data is wrapped by Result.Success. This creates a burden on the caller to check for success (or error) and unwrap the data. Example:

 tasksRepository.getTask(taskId).let { result ->
                if (result is Success) {
                    val task = result.data

By unwrapping the data (by removing Result), and using null to indicate that a task doesn't exist, which was a common error scenario in this codebase, we can simplify this to:

tasksRepository.getTask(taskId).let { task ->
                if (task != null) {

In (the pretty rare) situations where reading data can result in an error we can throw an exception, and handle it in the collector using a catch block, like this:

 tasksRepository.getTaskStream(taskId)
        .map { handleResult(it) }
        .catch {
            // error handling 
        }

which again is more readable than having when (result) -> is Success { ... } -> is Error { ... } inside the flow transformation functions e.g. map.

In summary, throwing exceptions is preferable to having a wrapper type because it avoids excessive code branching and data unwrapping.

johnjohndoe commented 1 year ago

Thank you for the explanation. I do not have insights into the codebase but I was asking myself why would a task but exist in the first place, motivating you to indicate the lack with null.?