5
\$\begingroup\$

I would like any constructive comments regarding the structure of this simple App that takes an API response and then displays on a table view.

The URL is written in a ConstantsAPI file

let baseUrl : String = "https://haveibeenpwned.com/api/v2"
let breachesExtensionURL : String = "/breaches"

Displayed on a tableviewcontroller

class SitewideTableViewController: UITableViewController, DataManagerDelegate {
    var pwnedData = [BreachModel]()
    var session: URLSession!
    var task: URLSessionDownloadTask!

    override func viewDidLoad() {
        super.viewDidLoad()
        session = URLSession.shared
        task = URLSessionDownloadTask()
        DataManager.shared.delegate = self
        DataManager.shared.fetchBreaches()
    }

    func didDownloadBreaches() {
        DispatchQueue.main.async {
            self.pwnedData = DataManager.shared.sortedBreaches()
            self.tableView.reloadData()
        }
    }

    override func tableView(_ tableView: UITableView, numberOfRowsInSection section: Int) -> Int {
        return pwnedData.count
    }

    override func tableView(_ tableView: UITableView, cellForRowAt indexPath: IndexPath) -> UITableViewCell {
        let cell = tableView.dequeueReusableCell(withIdentifier: "Sitewide", for: indexPath)
        cell.textLabel?.text = pwnedData[indexPath.row].name
        return cell
    }
}

Using the following model

import Foundation

class BreachModel : Codable {
    let name : String
    let title : String
    let domain : String
    let breachDate : String
    let addedDate : String
    let modifiedData : String
    let pwnCount : Int
    let description: String

    private enum CodingKeys: String, CodingKey {
        case name = "Name"
        case title = "Title"
        case domain = "Domain"
        case breachDate = "BreachDate"
        case addedDate = "AddedDate"
        case modifiedData = "ModifiedDate"
        case pwnCount = "PwnCount"
        case description = "Description"
    }
}

With a Data manager that would manage all of the data

@objc protocol DataManagerDelegate: class {
    // optional delegate to practice
    @objc optional func didDownloadBreaches() // called when the manager has completed downloading all the breaches
}


    class DataManager {
        static let shared: DataManager = DataManager()
        public weak var delegate: DataManagerDelegate? = nil

        private var breaches = [BreachModel]()
        func fetchBreaches() {

            HTTPManager.shared.get(urlString: baseUrl + breachesExtensionURL, completionBlock:  { [weak self] (data: Data?) -> Void in
                let decoder = JSONDecoder()
                if let data = data{
                    print(data.count)
                    do {
                        self?.breaches = try decoder.decode([BreachModel].self, from: data)
                        self?.delegate?.didDownloadBreaches?()
                    } catch let error {
                        print ("Error in reading data", error)
                    }
                }
                }
            )
        }

        func sortedBreaches() -> [BreachModel] {
            return breaches.sorted{ a,b in a.name < b.name }
        }
    }

That calls a HTTP manager whose only responsibility is to call url's

class HTTPManager {
    static let shared: HTTPManager = HTTPManager()
    public func get (urlString: String, completionBlock: ((Data?) -> Void)?) {
        let url = URL(string: urlString)
        if let usableUrl = url {
            let request = URLRequest(url: usableUrl)
            let task = URLSession.shared.dataTask(with: request, completionHandler: { (data, response, error) in
                completionBlock?(data)
            })
            task.resume()
        }
    }
}

So is this a reasonable extendable structure? Should I have used dataTask or URLSessionDownloadTask? Have I unwittingly introduced some memory leaks?

Any comments appreciated, I'm familiar with the Swift book but find other tutorials tend to just show the use of codable or an API and do not talk through the whole structure (at least not at an appropriate level for me). The code above does work, and I'm thinking of building upon it in the future but want to follow some form of best practice (no matter how trivial).

Git link: https://github.com/stevencurtis/basicnetworking

Improve this question
\$\endgroup\$

1 Answer 1

Reset to default
3
+50
\$\begingroup\$

My only major observation is the choice of DataManager:

  1. You’ve made the DataManager a singleton but it has a delegate. That means you can effectively only have one controller acting as the delegate for the DataManager. If you’re only going to have one delegate, then DataManager shouldn't be a singleton (so that every controller can have its own DataManager with its own delegate). Or, if you want to make it a singleton, then perhaps rather than delegate pattern, I might suggest completion handler pattern (which is what I did in my code at the end of this answer).

A few other observations, all fairly trivial in nature:

  1. I don’t know why SitewideTableViewController has session and task properties. You’re not using them and they don’t belong in view controller anyway.

  2. Even if you had a need for the task property, instantiating it to a blank URLSessionDataTask() is not a good practice.

  3. If you are going to make a view controller conform to some delegate protocol, I’d advise doing it in an extension to the class:

    class SitewideTableViewController: UITableViewController { ... }

    and

    extension SitewideTableViewController: DataManagerDelegate {
    func didDownloadBreaches() { ... }
}

    This keeps your code better organized. That having been said, I wouldn’t even use the delegate-protocol pattern.

  4. I’m not sure why the closure to HTTPManager’s get method is optional. You’re not going to be calling get unless you wanted the result passed back in the closure.

    class HTTPManager {
    static let shared: HTTPManager = HTTPManager()

    public func get(urlString: String, completionBlock: @escaping (Data?) -> Void) {
        guard let url = URL(string: urlString) else { return }

        let task = URLSession.shared.dataTask(with: url) { data, _, _ in
            completionBlock(data)
        }
        task.resume()
    }
}

  5. Going a step further, with a completion handler closure with a type of Data? you only know if it succeeded or failed, but not why it failed. I’d suggest you have this pass back the Data if successful, but an Error if not successful.

    A nice approach to this is to use a Result-based parameter to the closure. This is included in Swift 5, but in prior versions of Swift, you can define it yourself like so:

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

    Then, rather than returning just Data? (where nil means that there was some error, but you don’t know what the issue was), you can return a Result<Data, Error>. I’d also add some validation logic:

    class HTTPManager {
    static let shared: HTTPManager = HTTPManager()

    enum HTTPError: Error {
        case invalidURL
        case invalidResponse(Data?, URLResponse?)
    }

    public func get(urlString: String, completionBlock: @escaping (Result<Data, Error>) -> Void) {
        guard let url = URL(string: urlString) else {
            completionBlock(.failure(HTTPError.invalidURL))
            return
        }

        let task = URLSession.shared.dataTask(with: url) { data, response, error in
            guard error == nil else {
                completionBlock(.failure(error!))
                return
            }

            guard
                let responseData = data,
                let httpResponse = response as? HTTPURLResponse,
                200 ..< 300 ~= httpResponse.statusCode else {
                    completionBlock(.failure(HTTPError.invalidResponse(data, response)))
                    return
            }

            completionBlock(.success(responseData))
        }
        task.resume()
    }
}

    Then fetchBreaches can do:

    func fetchBreaches() {
    HTTPManager.shared.get(urlString: baseUrl + breachesExtensionURL) { [weak self] result in
        switch result {
        case .failure(let error):
            // handle error here

        case .success(let data):
            // process `Data` here
        }
    }
}

  6. I’d suggest using Date types in BreachModel (which I’d personally just call Breach and make it a struct):

    struct Breach: Codable {
    let name: String
    let title: String
    let domain: String
    var breachDate: Date? { return Breach.dateOnlyFormatter.date(from: breachDateString) }
    let breachDateString: String
    let addedDate: Date
    let modifiedDate: Date
    let pwnCount: Int
    let description: String

    private enum CodingKeys: String, CodingKey {
        case name = "Name"
        case title = "Title"
        case domain = "Domain"
        case breachDateString = "BreachDate"
        case addedDate = "AddedDate"
        case modifiedDate = "ModifiedDate"
        case pwnCount = "PwnCount"
        case description = "Description"
    }

    static let dateOnlyFormatter: DateFormatter = {
        let formatter = DateFormatter()
        formatter.locale = Locale(identifier: "en_US_POSIX")
        formatter.dateFormat = "yyyy-MM-dd"
        return formatter
    }()
}

    The only trick here is that this API returns BreachDate as a date-only string, but AddedDate and ModifiedDate as date time strings. So, I’d use the standard ISO8601 date formatter for the decoder’s dateDecodingStrategy (shown below) for the latter two, but lazily decode the BreachDate using a date-only date formatter.

    The decoder would then look like:

    let decoder = JSONDecoder()
decoder.dateDecodingStrategy = .formatted(ApiManager.dateTimeFormatter)

do {
    let breaches = try decoder.decode([Breach].self, from: data)
    // use `breaches` here
} catch {
    // handle `error` here
}

    where

    static let dateTimeFormatter: DateFormatter = {
    let formatter = DateFormatter()
    formatter.locale = Locale(identifier: "en_US_POSIX")
    formatter.dateFormat = "yyyy-MM-dd'T'HH:mm:ssX"
    formatter.timeZone = TimeZone(secondsFromGMT: 0)
    return formatter
}()

    But by making the model type use proper Date objects, then you can format them nicely in your UI without littering your UI code with logic to convert the ISO 8601 strings to dates.

  7. I’d personally pull the sorting of dates out of the DataManager and put it in Breach.swift in an extension to Array (or RandomAccessCollection):

    extension RandomAccessCollection where Element == Breach {
    func sortedByName() -> [Breach] {
        return sorted { a, b in a.name < b.name }
    }
}

    Then, when you have your array of breaches, you can just do

    self.pwned = breaches.sortedByName()

  8. I notice that your pwnedData (which I might suggest renaming to pwnedBreaches because it’s an array of Breach objects, not an array of Data objects) is initialized as an empty [BreachModel] before you retrieve the data. It’s not terribly critical in this particular case, but as a general rule, it is useful to distinguish between “this property has not been set” and “it has been set but there are no records.”

    Bottom line, I’d suggest making this an optional (where nil means that it hasn’t been set yet, and [] means that it has been set to an empty array).

So pulling that all together, we end up with something like:

// SitewideTableViewController.swift

class SitewideTableViewController: UITableViewController {
    var pwnedBreaches: [Breach]?

    override func viewDidLoad() {
        super.viewDidLoad()

        ApiManager.shared.fetchBreaches { [weak self] result in
            guard let self = self else { return }

            switch result {
            case .failure(let error):
                print(error)

            case .success(let breaches):
                self.pwnedBreaches = breaches.sortedByName()
                self.tableView.reloadData()
            }
        }
    }
}

// MARK: - UITableViewDataSource

extension SitewideTableViewController {
    override func tableView(_ tableView: UITableView, numberOfRowsInSection section: Int) -> Int {
        return pwnedBreaches?.count ?? 0
    }

    override func tableView(_ tableView: UITableView, cellForRowAt indexPath: IndexPath) -> UITableViewCell {
        let cell = tableView.dequeueReusableCell(withIdentifier: "Sitewide", for: indexPath)
        cell.textLabel?.text = pwnedBreaches?[indexPath.row].name
        return cell
    }
}

// Breach.swift

struct Breach: Codable {
    let name: String
    let title: String
    let domain: String
    var breachDate: Date? { return Breach.dateOnlyFormatter.date(from: breachDateString) }
    let breachDateString: String
    let addedDate: Date
    let modifiedDate: Date
    let pwnCount: Int
    let description: String

    private enum CodingKeys: String, CodingKey {
        case name = "Name"
        case title = "Title"
        case domain = "Domain"
        case breachDateString = "BreachDate"
        case addedDate = "AddedDate"
        case modifiedDate = "ModifiedDate"
        case pwnCount = "PwnCount"
        case description = "Description"
    }

    static let dateOnlyFormatter: DateFormatter = {
        let formatter = DateFormatter()
        formatter.locale = Locale(identifier: "en_US_POSIX")
        formatter.dateFormat = "yyyy-MM-dd"
        return formatter
    }()
}

extension RandomAccessCollection where Element == Breach {
    func sortedByName() -> [Breach] {
        return sorted { a, b in a.name < b.name }
    }
}

// Result.swift
//
// `Result` not needed if you are using Swift 5, as it already has defined this for us.

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

// ApiManager.swift

class ApiManager {
    static let shared = ApiManager()

    let baseUrl = URL(string: "https://haveibeenpwned.com/api/v2")!
    let breachesExtensionURL = "breaches"

    static let dateTimeFormatter: DateFormatter = {
        let formatter = DateFormatter()
        formatter.locale = Locale(identifier: "en_US_POSIX")
        formatter.dateFormat = "yyyy-MM-dd'T'HH:mm:ssX"
        formatter.timeZone = TimeZone(secondsFromGMT: 0)
        return formatter
    }()

    func fetchBreaches(completion: @escaping (Result<[Breach], Error>) -> Void) {
        let url = baseUrl.appendingPathComponent(breachesExtensionURL)

        HTTPManager.shared.get(url) { result in
            switch result {
            case .failure(let error):
                DispatchQueue.main.async { completion(.failure(error)) }

            case .success(let data):
                let decoder = JSONDecoder()
                decoder.dateDecodingStrategy = .formatted(ApiManager.dateTimeFormatter)

                do {
                    let breaches = try decoder.decode([Breach].self, from: data)
                    DispatchQueue.main.async { completion(.success(breaches)) }
                } catch {
                    print(String(data: data, encoding: .utf8) ?? "Unable to retrieve string representation")
                    DispatchQueue.main.async { completion(.failure(error)) }
                }
            }
        }
    }
}

class HTTPManager {
    static let shared = HTTPManager()

    enum HTTPError: Error {
        case invalidResponse(Data?, URLResponse?)
    }

    public func get(_ url: URL, completionBlock: @escaping (Result<Data, Error>) -> Void) {
        let task = URLSession.shared.dataTask(with: url) { data, response, error in
            guard error == nil else {
                completionBlock(.failure(error!))
                return
            }

            guard
                let responseData = data,
                let httpResponse = response as? HTTPURLResponse,
                200 ..< 300 ~= httpResponse.statusCode else {
                    completionBlock(.failure(HTTPError.invalidResponse(data, response)))
                    return
            }

            completionBlock(.success(responseData))
        }
        task.resume()
    }
}
Improve this answer
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.