I originally wrote this on the ios-developers slack #code-review channel, and a few people suggested I make it a blog post. I wasn't really interested in doing that, code reviews aren't really a subject I write about. But they are super important, so here goes.

TL;DR - there's a gist

Let's start with the original code

That's a lot to take in all at once but it's just a standard download-and-parse bit of code that we've all seen before.

The code isn’t broken, it does the job and that's always half the battle. But I had the following observations regarding the number of responsibilities contained in a single class:

Was there really any need for a shared instance?
Converting Data objects to an internal representation of an array of dictionaries
Converting the array of dictionaries to an array of Contacts
Network access
Knowledge of the network address
If I want to download and parse something else from another location, I have to repeat a lot of that code. I definitely don't want to do that. I want something more extensible so I don't repeat myself.

So here's what I'd do

Split everything out into small units with fewer responsibilities
Make everything less reliant on specific types (in this case the Contact class) in order to make everything more generic.

In the end, we'll see how much is left of the ContactInfo class.

First things first, [String: Any] appears everywhere in the code, I don't want to keep writing that, so I'll start with a typealias to reduce the clutter, and maybe it'll make things more readable in the process.

typealias JSONFormat = [String: Any]

And I'll keep the error enum as it is.

Next, the Contact class/struct. I know it can be constructed using JSONFormat. But then again, any class or struct that I download in JSON form should also be constructed in the same way, so I'll create a protocol to make this explicit

protocol JSONConstructible {
  init?(_ json: JSONFormat) 
}

And define Contact as conforming to this protocol.

struct Contact: JSONConstructible {
  init?(_ json: JSONFormat) {
    // Contact stuff goes here...
  }
}

Now when I want to think in terms of Contact, I'll think about JSONConstructible instead, keeping things general.

Note: I've made it a failable initialiser rather than specifying that it throws. I'll explain why later.

Now for some constants. I don't need to specify the URLs in ContactInfo directly, I can just add them as an extension to URL. This is just a personal preference of mine, YMMV.

extension URL {
  private static let baseURL = "http://thingywotsit.com"
  static let contacts = URL(string: "(URL.baseURL)/users")!
}

Let's get down to some code. I don't want ContactInfo to be responsible for converting Data to JSONFormat. I'll make that an extension on Data itself.

extension Data {
  func convertJSON() throws -> [JSONFormat] {
    guard let serializedJson = try JSONSerialization.jsonObject(with: self) as? [JSONFormat] else {
      throw JSONParsingError.convertError
    }    
    return serializedJson
  }
}

Ok, that's the URL location and conversion responsibilities out of the way, and we remove them from ContactInfo ✅.

Next we look at the parsing responsibility. We have an array being returned from convertJSON, so we can add an extension on Array to parse its contents into whatever things the JSON should become, and I don't even know what it's going to become, so I'm going to be generic here.

extension Array where Element == JSONFormat { // 1
  func parseJSON<T: JSONConstructible>() -> [T] { // 2
    return flatMap(T.init) // 3
  }
}

Very few lines of code, with a lot going on.

This extension only applies to arrays that contain JSONFormat elements.
The parseJSON method only applies to things that conform to the JSONConstructible protocol defined earlier.
We know JSONConstructible has an init method that takes JSONFormat, so we can map the contents of the array to the JSONConstructible initialiser, that's what the flatMap is doing.

I said before I wanted init to be failable, and this is why, flatMap will filter out all objects that return nil. This way I can make sure the maximum number of elements in the array are converted to my model types, and if there were some corruptions that caused one or two to fail, no biggie. If I used exceptions here I'd lose everything, I'd rather not do that. But again, YMMV.

And now we've removed parsing from ContactInfo ✅.

Deep breath, downloading. I want to download data and convert it to Contact, or more generally to any JSONConstructible type. Again, I need to use generics

struct JSONDownloader {
  static func download<T: JSONConstructible>( // 1
        url: URL,
        completion: @escaping (Result<[T]>) -> Void) { // 2

    URLSession.shared.dataTask(with: url) { (data, response, error) in
      guard
        let data = data,
        let response = response as? HTTPURLResponse,
        response.statusCode == 200
      else {
        completion(.failure(error))
        return
      }

      do {
        let jsonArray = try data.convertJSON() // 3
        let types: [T] = jsonArray.parseJSON() // 4

        completion(.success(types))
      }
      catch let error {
        completion(.failure(error))
      }
    }.resume()
  }
}

My downloader is again using generics and the generic type must conform to JSONConstructible.

The placeholder T has to conform to JSONConstructible.
I'm using the Result enum that we all know and love.
I call convertJSON directly on the data object returned in the response, getting back an array of JSONFormats.
I parse the array, specifying that the result is of type [T], which is the same type passed to the downloader. So parseJSON knows what type to infer as well.

And now we've moved downloading out of ContactInfo ✅.

So, how do we get our hands on a list of Contacts? It's super simple

JSONDownloader.download(url: .contacts) { (result: Result<[Contact]>) in
  // ...
}

We call the downloader, with the correct URL, and give the completion block the correct types, in this case Contact, which will make the compiler correctly figure out the type to use in the generic methods in JSONDownloader and parseJSON.

You might be asking: isn't this a huge refactor with very little gain? I mean, the original code was doing the same thing and didn't need all that generic stuff. Isn't this just over engineering?

Well, could be. Especially if you know you'll only ever have one parsed type. But what if we now have a new type that we want to download? If we have a new Widget type and want to implement downloading, converting and parsing, all we have to do is make Widget conform to JSONConstructible, supply the right URL and off we go:

extension URL {
  private static let baseURL = "http://thingywotsit.com"
  static let contacts = URL(string: "(URL.baseURL)/users")!
  static let widgets = URL(string: "(URL.baseURL)/widgets")!
}

struct Widget: JSONConstructible {
  init?(_ json: JSONFormat) {
    // ...
  }
}

JSONDownloader.download(url: .widgets) { (result: Result<[Widget]>) in
}

Now we have that extra thing I was looking for: extensibility. And without repeating any code. Not just extensibility in this project, but I can use this code in any project I want now.

And another thing we have after the refactor is testability. I can add tests to Data to check the convertJSON method works as expected. Same for testing parseJSON in Array. I couldn't do that previously without calling a whole download operation.

There is still some work to be done to make the downloader testable, though. But that's a whole other blog post.

Final thought, now that I've refactored a lot of the responsibilities out into separate areas, and removed the need for the shared instance, what does ContactInfo look like? It's this:

class ContactInfo: NSObject {
}

It turns out, for all the responsibilities it was given, it wasn't needed at all.