Replace Result with exception handling

[dturner]

Here's what I've done:

• Fixed a couple of unit tests where the behaviour was broken, specifically in DefaultTaskRepositoryTest
• Replaced all instances of Result.Error with throw Exception
• Added catch blocks to flow collection and added a couple of user error messages to indicate to them what went wrong
• Removed the Result class
• Changed the return type of getTask from Result<Task> to Task?, returning null indicates that the Task doesn't exist
• Updated unit tests to verify new behaviour

[johnjohndoe]

Can you please explain why you made the changes?

[dturner]

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]

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.?

[manuelvicnt]

Thanks! Great work!

A good data point that shows how the Result type in the data layer is complicating the Arch is the file changes data (i.e. +198 - 332). You're removing complexity, yay!

I think the Result class was an overkill for this project. Other apps might benefit from it but definitely not this one :)