Basic network layer for a JSON API client

Question

Just looking for some feedback/ways to improve this basic networking layer written in Swift. I'm still learning, please elaborate with as much detail as possible.

enum HTTPMethod: String {
    case get = "GET"
}

 class HTTPLayer {

     let baseURL = URL(string: "https://example.com")!

     func request(at endpoint: Endpoint, completion: @escaping (Data?, Error?) -> Void) {
        let url = baseURL.appendingPathComponent(endpoint.path)
        let task = URLSession.shared.dataTask(with: url) { (data, response, error) in
            completion(data, error)
        }
        task.resume()
    }

 }

 enum Endpoint {

     case listTransactions
    case transactionDetails(String)

     var method: HTTPMethod {
        return .get
    }

     var path: String {
        switch self {
        case .listTransactions:
            return "/api/transactions/"
        case .transactionDetails(let id):
            return "/api/transactions/id"
        }
    }

 }

 struct Transaction: Decodable {
    let id: String
    let amount: Double
}

 struct TransactionDetails: Decodable {
    let payload: [String: String]
}

 enum Result<T> {
    case success(T)
    case failure(NSError)
}

 class APIClient {

     let httpLayer: HTTPLayer
    init(httpLayer: HTTPLayer) {
        self.httpLayer = httpLayer
    }

     func transactions(completion: @escaping (Result<[Transaction]>) -> Void) {
        self.httpLayer.request(at: .listTransactions) { (data, error) in
            if error != nil {
                completion(.failure(error! as NSError))
            } else {
                do {
                    let transactions = try JSONDecoder().decode([Transaction].self, from: data!)
                    completion(.success(transactions))
                } catch {
                    completion(.failure(self.defaultError))
                }
            }
        }
    }

     func transactionDetails(for transaction: Transaction, completion: @escaping (Result<TransactionDetails>) -> Void) {
        self.httpLayer.request(at: .transactionDetails(transaction.id)) { (data, error) in
            if error != nil {
                completion(.failure(error! as NSError))
            } else {
                do {
                    let transactions = try JSONDecoder().decode(TransactionDetails.self, from: data!)
                    completion(.success(transactions))
                } catch {
                    completion(.failure(self.defaultError))
                }
            }
        }
    }

     var defaultError: NSError = NSError(domain: "", code: 1, userInfo: nil)

 }

Answer 1

There are few things that can be improved, will start with the major ones:

General

• Instead of having function for each API call you can generalise it a bit like so:

/* Updated the name, since it's not only for transactions anymore
Since we no longer know the exact endpoint we will have to pass it to the function

I would also add `requestMeta` parameter that will contain any other information that would be needed to perform the api call
`parameters`, `encoding`, `authenticationType`, `additionalHeaders`, `shouldReauthenticate`
but this is in case you need them
*/

func apiReques<T: Codable>(_ dataType: T.Type,
                                    endpoint: Endpoint,
                                  completion: @escaping (Result<[T], ResultError>) -> Void) {

        self.httpLayer.request(at: endpoint) { (data, error) in

// In general an "early return" strategy is preferred, use guard when applicable and avoid nesting `if` statements
            if let error = error as NSError? {
                completion(.failure(error))
                return
            }

// Do not force unwrap, it's better to show empty screen (or error) instead of crashing the app
            guard let data = data else {
                completion(.failure(NSError(domain: "", code: 1, userInfo: [:])))
                return
            }

            do {
                let transactions = try JSONDecoder().decode([T].self, from: data)
                completion(.success(transactions))
            } catch {
                completion(.failure(self.defaultError))
            }
        }
    }

So, since your structures are Decodable (consider using Codable) you can easily use generics.

The _ dataType: T.Type is a bit tricky. Since we are not returning T we have to add it as a parameter so the compiler can infer the actual type (more on the topic can be found here

• I would prefer keeping all the urls in one place, so I would move baseURL to endpoints enum. Again do not force unwrap

enum Endpoint {

    static let baseURLString = "https://example.com"

    case listTransactions
    case transactionDetails(String)
}

extension Endpoint {

    var method: HTTPMethod {
        return .get
    }

    var path: String {
        switch self {
        case .listTransactions:
            return Endpoint.baseURLString + "/api/transactions/"
        case .transactionDetails(let id):
            return Endpoint.baseURLString + "/api/transactions/\(id)"
        }
    }
}

This will affect your request func:

func request(at endpoint: Endpoint, completion: @escaping (Data?, Error?) -> Void) {

        guard let url = URL(string: endpoint.path) else { return }
// endpoint.method is available as well if needed

        let task = URLSession.shared.dataTask(with: url) { (data, response, error) in
            completion(data, error)
        }
        task.resume()
    }

• It seems that you are instantiating new HTTPLayer for every APIClient. In that case I would just go with private let httpLayer: HTTPLayer = HTTPLayer() and deleting the init. In that case you can mark class HTTPLayer as fileprivate so noone beside APIClient can access it and if any params needs to be passed to it will go through APIClient -> apiReques()

Error handling

• If you want to go a bit further you can update the enum Result

enum Result<Value, Error: Swift.Error> {
    case success(Value)
    case failure(Error)
}

and also adding your custom errors:

enum ResultError: Error {
    case general(String?)
    case specific(Int?)
    case `default` // default is a keyword so if you want to use it, you should wrap it in ``, imo don't and choose another name
    case whateverYouWant
}

extension ResultError {
    var errorString: String {
        switch self {
...
        }
    }

    private func specificErrorString(_ errorCode: Int) -> String {
        switch errorCode {
...
        }
    }
}

This will affect the generic apiRequest a bit in the signature @escaping (Result<[T], ResultError>) -> Void) and ofc the completions

completion(.failure(.general("something went wrong")))
completion(.failure(.specific(123))
completion(.failure(.default)

Now you can delete var defaultError: NSError = NSError(domain: "", code: 1, userInfo: nil)

How to use it

APIClient(). apiReques(Transaction.self,endpoint: Endpoint.listTransactions) { data in

    switch data {
    case .success(let response):
        print("Success")
    case .failure(let error):
        print("error")
    }
}

Bonus

You can also split the completion into two completion success and failure

• Signature

func apiReques <T: Decodable>(_ dataType: T.Type,
                                    endpoint: Endpoint,
                                    success: @escaping (([T]) -> Void),
                                    failure: @escaping ((ResultError) -> Void))

• block calls

failure(.general(""))
failure(.default)
success(data)

• Usage

APIClient().transactions(Transaction.self,
                         endpoint: Endpoint.listTransactions,
                         success: { data in
                            print(data)
}, failure: { error in
    print(error)
})