Improve Either type

[manavgabhawala]

The Either enum type is at the heart of everything that happens with the network and model objects as well as error handling, but there is definitely room for improvement. As of now its defined as such:

enum Either<T>
{
    case Value(T)
    case NetworkingError(NSError)
    case UnknownError
}

This works great but there are some drawbacks. For instance, UnknownError doesn't provide any information about what the error might be and NetworkingError might be a bit too restrictive. There are two possible improvements I see:

1. This is less disruptive to the code base but still allows us to allow for better error handling of UnknownError. This would simply be to change to:

enum Either<T>
{
    case Value(T)
    case NetworkingError(NSError)
    case UnknownError(String)
}

and pass along a string of information wherever we throw the .UnknownError. This could then be wrapped along and sent to our error handler.

2. Our other option is to change to something like:

enum Either<T>
{
    case Value(T)
    case Error(NSError) // Instead of NSError we could use ErrorType here maybe?
                        // Extracting a string with information would be awkward though
    case UnknownError
}

And then trap on UnknownError (where it indicates a runtime programmer error in the code instead of an actual error, i.e. this kind of error should never be thrown in the wild, if it is something is horribly wrong!) and then use the more generic .Error everywhere else (i.e. open up NetworkingError). This approach would take considerable more effort to clean up though.

Personally, I prefer number 2. Open to other suggestions.

[russellladd]

As long as we're in favor of rewiring the error system, I propose this as a cleaner alternative to both solutions. The crux is that the new "Either" type (renamed Response) only has two cases, because NSError and any custom error all conform to ErrorType.

//: Playground - noun: a place where people can play

import UIKit

// Simplified "either type"
// Renamed because a true Either should have two generic parameters
enum Response<T> {
    case Success(T)
    case Error(ErrorType)
}

// Per Swift 2 error handling guidelines, we should define our errors with enums
// We could back by strings for explanations here, or even associated values
enum MHacksError: ErrorType {
    case CustomError1
    case CustomError2
}

// An example function that takes a callback to return its response
func getDataFromWeb(parameter: Int, completionBlock: (Response<String>) -> ()) {

    switch parameter {
    case 0:
        completionBlock(.Success("Here is some data!"))
    case 1:
        completionBlock(.Error(MHacksError.CustomError1))
    case 2:
        completionBlock(.Error(NSError(domain: NSCocoaErrorDomain, code: 0, userInfo: [NSLocalizedDescriptionKey: "This failed for some reason"])))
    default:
        assertionFailure()
    }
}

let completionBlock = { (either: Response<String>) in
    switch either {
    case .Success(let data):
        print(data)
    case .Error(let error):
        print(error)
    }
}

getDataFromWeb(0, completionBlock: completionBlock) // Here is some data!
getDataFromWeb(1, completionBlock: completionBlock) // MHacksError.CustomError1
getDataFromWeb(2, completionBlock: completionBlock) // ErrorDomain=NSCocoaErrorDomain Code=0 "This failed..."

[manavgabhawala]

@russellladd your suggestion is pretty similar to number 2 with the omission of UnknownError. I found UnknownError helpful when testing out new code without having to set breakpoints and such (especially cause we were figuring out how the backend worked). fatalError() is always an option to avoid the need for it though. In any case with regards to your other error handling the enum would be great if we knew the list of possible errors beforehand. Unfortunately I can't think of a place where we do, most of them are dynamic HTTP errors or JSON deserialization errors that's why NSError was easy to work with. Also like I said extracting a String from CustomError1 would be a bit clunky or would require a massive switch case somewhere. Another way of doing things might be to use a new type:

protocol MHacksError: ErrorType {
    var localizedError: String { get }
}

Then make NSError and any ErrorTypes we declare conform to that instead. And then to extract the string we pull it out by casting to MHacksError and then get the localizedError.

[russellladd]

Errors in Swift should always have the semantic of being recoverable. Therefore, an UnknownError for debugging purposes doesn't make any sense to me. fatalError should be used for errors that should always crash the program, even if they make it into shipping code. assertionFailure is a similar function (and the one we should mostly be using) as it is designed for catching developer bugs that should not make it into shipping code - the benefit of this over fatalError is that these check are removed in -Ounchecked optimized builds.

@manavgabhawala I don't understand your reasoning for why my enum Response is insufficient. HTTP errors and JSON errors can just be passed directly into the .Error() constructor. For errors we define, there must be a defined list because we must have a predefined number of possible errors. If some of our errors have dynamic data associated with them (including an underlying NSError), it can be added to that errors case as an associated value.

[manavgabhawala]

As for the UnknownError I agree, we should stop using it. It was helpful to not crash for when the backend was very unstable and changing a lot. (In fact that was the main reason I created an issue out of this was to migrate away from this "anti pattern")

I think that's enough, all I'm saying is we display errors to the user (see AppDelegate.swift) and the way we do it is with a combination of NSNotification and extracting a string from NSError. The error enum won't work with both NSNotification and extracting of a string, so I think its best to just stick to using NSError. There are several places we could make things more Swifty but don't because of objc limitations, like with NSCoding too.

[manavgabhawala]

Resolved with 969bd6bbca91ea3a32ead4f195b86ec8000335c9