How to refactor massive view controllers

Learn hands-on techniques for better architecture

Paul Hudson    April 7th 2019    @twostraws

I’ve stood on stage a number of times and talked about the importance of having a smart, scalable architecture. However, that’s easier said than done: most apps start with a rather clumsy proof of concept, and often some if not all of that initial code makes it into the final release.

We optimistically refer to such code as “technical debt”, giving life to the assumption that we’re actually going to pay down that debt at some point. All too often, though, that doesn’t happen: we’re too busy keeping up with new iOS releases, new devices, and new feature requests to focus on cleaning up old code.

Easily the biggest problem in our community is massive view controller syndrome: where we add more and more functionality to view controllers, largely because that’s the easiest place to put it. Previously I’ve talked about this problem in separate articles, but I figured now was the time to put them all together, add some more, and produce one single guide.

So, in this article we’re going to walk through a variety of techniques you can apply to your code, all using the same project – you’ll literally be able to watch the project get better and better as we progress. Even better, you can apply these same techniques to the code in your own project. Yes, this does mean carving out a little time to work on technical debt, but I hope you’ll see it’s worth the effort!

This article is based on a talk I gave in Turin. I ran out of time during that presentation, so consider this article the “director’s cut” as it were – all the original, plus bonus content that I couldn’t originally fit in!

Sponsor Hacking with Swift and reach the world's largest Swift community!

What do we have?

I’ve built an app that demonstrates a number of problems, all of which result in massive view controllers. The best place to start is for you to download the project, run it in the simulator, and browse through the code.

You can get the source code here.

Start by running the app and using it a little. You’ll see it’s an app that lists 30 of the original Hacking with Swift projects. Choosing a project shows more information, and if you tap to read the project you’ll see a small, embedded web view. This isn’t a complicated project, but it’s not designed to be.

Now look over the code for each view controller in the project: ViewController.swift has about 74 lines, DetailViewController.swift has about 91 lines, and ReadViewController.swift has about 50 lines. For such a small app, these are some pretty large view controllers!

Centralizing code

Let’s start by looking for common functionality that we can move from view controllers.

The app starts in ViewController.swift, so let's start there too. What you’ll see is that viewDidLoad() has a big chunk of code for loading JSON from the bundle, which doesn’t need to be there – this is the kind of thing you might want to do in lots of places.

This view controller clearly needs bundle loading functionality, but it doesn’t need to own it – we could move that to an extension that the rest of our project can use. So, we’re going to start by extracting that code into something that can be used anywhere.

First, make a new Swift file called Bundle+Decoding.swift. Now select all the loading code in viewDidLoad() and cut it to the clipboard. That’s all this code:

guard let url = Bundle.main.url(forResource: "projects", withExtension: "json") else {
    fatalError("Failed to locate projects.json in app bundle.")
}

guard let data = try? Data(contentsOf: url) else {
    fatalError("Failed to load projects.json in app bundle.")
}

let decoder = JSONDecoder()

guard let loadedProjects = try? decoder.decode([Project].self, from: data) else {
    fatalError("Failed to decode projects.json from app bundle.")
}

In the new Bundle+Decoding.swift file, add this method, and paste in your clipboard where I added a comment:

extension Bundle {
    func decode<T: Decodable>(from filename: String) -> T {
        // paste clipboard here
    }
}

We need to do a little clean up to make that compile:

1. Change forResource: "projects", withExtension: "json" to forResource: filename, withExtension: nil.
2. Change all the strings with projects.json to \(filename).
3. Change guard let loadedProjects to guard let loaded because we could be loading anything.
4. Change [Project].self to T.self.
5. Return loaded at the end of the method.

Back in view controller, delete this line from viewDidLoad():

projects = loadedProjects

Instead, we can call our new decode() method straight from the property definition:

let projects: [Project] = Bundle.main.decode(from: "projects.json")

Not only has that removed a lot of code from our view controller, but it also turned projects from a variable to a constant – a small but important win.

Model to model

ViewController.swift contains some other shared functionality: the makeAttributedString() method. This is designed to take the title and subtitle of a project, and return them as an attributed string that can be used in table view cells.

This is interesting. It’s transforming one piece of model data into another piece of model data, which meant if we added more screens to this app – bookmarks, filters, or related projects, for example – we'd need to copy and paste this method. So, clearly a better idea is to move it someplace else, but where?

There are two commonly accepted options:

1. Create a view model wrapper around this model, which is able to formats the model data neatly.
2. Give our model the know-how to transform itself however it wants.

Either of those are good, but as we already have lots of other things to tackle I'm going to take the easiest one: we’re going to give the model an attributed string property. In case you were wondering, attributed strings are still model data, so this is really just a model knowing how to transform itself into a different model – there’s no view data here.

Anyway, cut the makeAttributedString() method to your clipboard, then paste it into the Project model. I don’t think this feels quite right as a method, so I would convert it to a computed property like this:

var attributedTitle: NSAttributedString {

We’ll get a compile error because we removed the makeAttributedString() method, so look for this line in the cellForRowAt method:

cell.textLabel?.attributedText = makeAttributedString(title: project.title, subtitle: project.subtitle)

Now change it to this:

cell.textLabel?.attributedText = project.attributedTitle

Carving off data sources

At this point, ViewController.swift is responsible for only two major things: acting as a table view data source, and acting as a table view delegate. Of the two, the data source is always an easier target for refactoring because it can be isolated neatly.

So, we’re going to take all the data source code out of ViewController.swift, and instead put it into a dedicated class. This allows us to re-use that data source class elsewhere if needed, or to switch it out for a different data source type as appropriate.

Go to the File menu and choose New > File. Create a new Cocoa Touch Class subclassing NSObject, and name it it “ProjectDataSource”. We’re going to paste a good chunk of code into here in just a moment, but first we need to do two things:

1. Add import UIKit to the top of the file, if it isn’t there already.
2. Make this new class conform to UITableViewDataSource.

It should look like this:

import UIKit

class ProjectDataSource: NSObject, UITableViewDataSource {

}

Now for the important part: we need to move numberOfSections, numberOfRows, and cellForRowAt into there, from ViewController.swift. You can literally just cut them to your clipboard then paste them inside ProjectDataSource, but you will need to remove the three override keywords.

Now, we're getting compiler warnings all over the place because this new class doesn't know what the projects property is – that’s still in ViewController.swift. We could make this load its own copy of the projects, or even pass one in from the view controller, but a better idea is to make this data source handle all the data for us. That is, make it the sole responsible place in our code for loading and managing our app’s data

So, move the projects property from ViewController to ProjectDataSource. That will get rid of most, but not all, of the errors.

We can now update the ViewController class so that it creates an instance of ProjectDataSource, and uses it for the table view data source.

First, create this new property in ViewController.swift:

let dataSource = ProjectDataSource()

Now add this to its viewDidLoad() method:

tableView.dataSource = dataSource

With that change we should be down to just one error: the didSelectRowAt method of ViewController needs access to the projects array so it can pass the selected project into DetailViewController.

Now, we could reach into the data source to read out projects, but a better idea is to create a simple getter method. So, add this to ProjectDataSource:

func project(at index: Int) -> Project {
    return projects[index]
}

Now back in the didSelectRowAt method we can write this:

let project = dataSource.project(at: indexPath.row)

That’s much cleaner, and allows our getter method to act as a gatekeeper for project data.

Even better, the ViewController started out at 74 lines and is now down to just 32 lines – we’ve taken it down by half. We'll come back to it later, but for now this is good enough.

Creating views in code

The next view controller we're going to look at is DetailViewController. This is about 91 lines of code right now, but it commits a cardinal sin in the way it creates its views in code.

Now, don’t get me wrong: I very often create views in code, and love the way it gives us complete control over every piece of our layout. However, there's a right way and a wrong way to create views in code, and DetailViewController chooses the wrong way: it puts view creation code in viewDidLoad().

That method is called at a specific time, and that time is pretty clearly encoded right in its name: it’s called when the view has finished loading. That means it’s the wrong place to start loading your view, which is what this project does.

Now, we could just move all that view creation code somewhere, but a better idea is to move it into a dedicated UIView subclass. That’s the whole point of the V part in MVC: a dedicated view layer with its own code, rather than stuffing that code elsewhere.

So, cut to your clipboard all the view creation code in viewDidLoad() – that’s everything from let scrollView = UIScrollView() down to the end of the method. Now create a new Cocoa Touch Subclass, subclassing UIView and naming it “DetailView”. When the new file opens for editing, create an initializer using the default init(frame:) initializer, then paste in the code from your clipboard:

override init(frame: CGRect) {
    super.init(frame: .zero)
    // paste your code here
}

It’s safe to use .zero for the frame because this view will actually take up all the space in its view controller – more on that later.

Swift’s subclassing rules force us to create an init(coder:) method so that our view can be created from a storyboard, but we don’t actually need to make that do anything. So, add this required initializer too:

required init?(coder aDecoder: NSCoder) {
    fatalError("init(coder:) is not supported.")
}

At this point we’re going to have quite a few compile errors, but none of them are hard to fix.

First, remove all the view. code from the code you pasted into DetailView.swift. So, rather than having view.addSubview(scrollView) just use addSubview(scrollView).

Second, we need to know what to fill our labels with. Right now we're using init(frame:) to create our view, but that isn't needed because this view will occupy all the space in its view controller. That means it doesn’t need a frame to size itself, and we won’t be adding Auto Layout constraints to it – it is the view for the view controller, so it will always resize to fill the space.

So, rather than using init(frame:) we can create a custom initializer. This will accept a Project instance as its parameter, which the view can then use to configure itself. Replace override init(frame:) with this:

init(project: Project) {

Now the labels work fine because they now have a project value to reference. However, we still have a problem with the button: it’s trying to call the readProject() method from DetailViewController, and we didn’t move that into the view.

We need a way to communicate from the view to the view controller when our button is tapped, and there are several options available to us:

1. Responder chain.
2. Delegate.
3. Closure.

The responder chain approach can work, but it takes quite a bit to implement properly. Delegates can also work, but the easiest of the three is to inject a closure to be called when the button is pressed. When working with only one or two callbacks I definitely prefer closures to delegates – they give us a lot more flexibility.

So, we're going to create our view with a readAction closure that we can call when the button is tapped.

First, add this property:

var readAction: (() -> Void)?

Now change init(project:) to this:

init(project: Project, readAction: @escaping() -> Void) {

Finally, modify the start of the initializer to this:

self.readAction = readAction
super.init(frame: .zero)
backgroundColor = .white

With that readAction closure in place, we can now connect that to our button by adding a method to DetailView that bridges the gap between the target/action pattern of buttons and our closure:

@objc func readProject() {
    readAction?()
}

And with that done we can use our DetailView class inside DetailViewController by adding this method:

override func loadView() {
    view = DetailView(project: project, readAction: readProject)
}

As a finishing touch, we can also remove @objc from the readProject() method in DetailViewController, because it isn't needed any more.

Carving off delegates

So far we have:

• Centralized some helper code.
• Split off a table view data source.
• Moved view code into a dedicated UIView subclass.

Next on our list of targets is ReadViewController, which acts as a simple web view wrapper – it allows only some URLs to be shown, to stop users from going anywhere online.

This behavior is controlled through the view controller acting as WKNavigationDelegate for its web view. If we split that code out – making a dedicated navigation delegate object – it means you can re-use the logic elsewhere in your app, and switch out or disable the restrictions to create something like “parental unlock” or similar.

So, create a new Cocoa Touch Subclass, subclassing NSObject and named “NavigationDelegate”.

Make the new class import WebKit, and conform to WKNavigationDelegate like this:

import UIKit
import WebKit

class NavigationDelegate: NSObject, WKNavigationDelegate {

}

Now cut and paste all of the decidePolicyFor code from ReadViewController into the new NavigationDelegate class. You’ll get an error because the code relies on the allowedSites array, so please move that too.

Because we now have a dedicated WKNavigationDelegate object, that work no longer needs to be done by ReadViewController, so you can remove its WKNavigationDelegate conformance.

Instead, we can add this property to the view controller:

var navigationDelegate = NavigationDelegate()

And assign it to the web view in the loadView() method:

webView.navigationDelegate = navigationDelegate

Having the navigation delegate as a variable means we can change our navigation rules at runtime just by swapping out that class – it’s much more flexible, while also reducing our view controller’s size.

While we're in ReadViewController, I want to fix a personal annoyance: loading a URL string into a web view takes far too many steps! I mean, just look at it:

guard let url = URL(string: "https://www.hackingwithswift.com/read/\(project.number)/overview") else {
    return
}

let request = URLRequest(url: url)
webView.load(request)

Create a URL, wrap that in a URLRequest, then pass that to the web view. I once saw Soroush Khanlou deliver a talk called You Deserve Nice Things, with his point being that it’s helpful to create extensions to wrap common behavior – you save time and avoid bugs.

So, in the spirit of deserving nice things let’s make a small WKWebView extension that can load URLs from strings, rather than having to do the URL/URLRequest double wrap.

First, copy the URL loading code to the clipboard. Second, create a new Swift file called WebView+LoadString.swift, and give it this code:

import WebKit

extension WKWebView {
    func load(_ urlString: String) {
        // paste your clipboard here
    }
}

We need to do a little clean up to make that code work. First, replace this line:

guard let url = URL(string: "https://www.hackingwithswift.com/read/\(project.number)/overview") else {

With this:

guard let url = URL(string: urlString) else {

Second, replace this line:

webView.load(request)

With this:

load(request)

Finally, add this code to the end of viewDidLoad() in ReadViewController.swift:

webView.load("https://www.hackingwithswift.com/read/\(project.number)/overview")

Much better!

Setting up coordinators

If we look back at our three view controllers now, you’ll see they are much smaller and much simpler – they don’t a lot of work any more. But there's still more we can do.

The main remaining chunks of ViewController and ReadViewController are responsible for navigation – you see code like this:

override func tableView(_ tableView: UITableView, didSelectRowAt indexPath: IndexPath) {
    let project = dataSource.project(at: indexPath.row)

    guard let detailVC = storyboard?.instantiateViewController(withIdentifier: "DetailViewController") as? DetailViewController else {
        return
    }

    detailVC.project = project
    navigationController?.pushViewController(detailVC, animated: true)
}

That means ViewController needs to know about, create, configure, and display DetailViewController, which is messy. This sort of code leads to dependency pyramids, where ViewControllerA has its dependencies plus everything required for ViewControllerB, ViewControllerC, and ViewControllerD` – it just pushes them down as it goes.

The best solution I’ve found to this problem is also from Soroush, and it’s called coordinators. They are dedicated objects responsible for our application flow, and mean that none of our view controllers know about the existence of any other view controller. It also makes it easier to make variations, such as for A/B testing or to cater for iPhone/iPad differences.

Implementing coordinators in an iOS app takes a little bootstrapping. I’ve already explained how to do it step by step in my article “How to use the coordinator pattern in iOS apps”, so I’m not going to repeat myself here – we’re just going to crack on with the code. If you want explanation for how it all works, please refer to that article!

The first step is to make a protocol defining what a coordinator looks like. So, make a new Swift file called Coordinator.swift, give it an import for UIKit, then add this code:

protocol Coordinator {
    var navigationController: UINavigationController { get set }
    var children: [Coordinator] { get set }
    func start()
}

We also need a concrete implementation of that to drive our little app, so please create another new Swift file, this time calling it MainCoordinator.swift. Give it an import for UIKit, then this code:

class MainCoordinator: Coordinator {
    var navigationController: UINavigationController
    var children = [Coordinator]()

    init(navigationController: UINavigationController) {
        self.navigationController = navigationController
    }

    func start() {
        // we'll write this soon
    }
}

We want that main coordinator to be given control when our app starts, which takes a little bit of boilerplate that is usually hidden by storyboards. Even though this app has storyboards, we don’t want them to be responsible for launching our app – that’s the job of our coordinators.

So, open Main.storyboard and delete the root navigation controller: we’ll be making that in code. Now go to the settings for your project and remove "Main" from Main Interface so that iOS doesn’t attempt to bootstrap through the storyboard.

Instead, we need to bootstrap the app by hand. This will be very familiar to folks who made apps before storyboards were introduced: we need to create a UIWindow at the correct size, then make it visible. This is also where we’re going to create our main coordinator.

So, open AppDelegate.swift and add this property:

var coordinator: MainCoordinator?

Now put this into the didFinishLaunchingWithOptions method:

let navController = UINavigationController()
coordinator = MainCoordinator(navigationController: navController)
coordinator?.start()

window = UIWindow(frame: UIScreen.main.bounds)
window?.rootViewController = navController
window?.makeKeyAndVisible()

All that causes our app to start using the main coordinator rather than the storyboard.

That sets our app to call start() on our main coordinator, but we haven’t actually filled in that method yet. What should the main coordinator do when it starts? Well, in this project it just needs to show the main view controller.

When working with coordinators I always use a simple protocol that lets me create view controllers from storyboards. This approach means I can do some UI in storyboards, some in code; whatever works best on for each view controller. Again, this approach is documented fully in my article about coordinators with iOS apps, so you can refer to there for more detail.

Create a new Swift file called Storyboarded.swift and give it this code:

import UIKit

protocol Storyboarded {
    static func instantiate() -> Self
}

extension Storyboarded where Self: UIViewController {
    static func instantiate() -> Self {
        let id = String(describing: self)
        let storyboard = UIStoryboard(name: "Main", bundle: Bundle.main)
        return storyboard.instantiateViewController(withIdentifier: id) as! Self
    }
}

That instantiate() method relies on our view controllers using their class name for their storyboard identifier, but if you look at Main.storyboard you’ll see that’s exactly how they are configured already.

With that protocol ready, the next step is to make all three of our view controllers conform to Storyboarded:

class ViewController: UITableViewController, Storyboarded
class DetailViewController: UIViewController, Storyboarded
class ReadViewController: UIViewController, Storyboarded

We can now implement the start() method in our main coordinator, so that it creates an instance of our ViewController class and shows it immediately:

func start() {
    let vc = ViewController.instantiate()
    navigationController.pushViewController(vc, animated: false)
}

Managing navigation flow

After all this work with coordinators, we’re actually no better off – we’ve just created more code. However, here comes the important part: we can now take navigation away from our view controllers and give that role to our coordinator instead.

First, we need a way for ViewController and DetailViewController to communicate back to their coordinator. So, add this property to both of those view controllers:

weak var coordinator: MainCoordinator?

We don’t need to give that property to ReadViewController because it doesn’t navigate anywhere.

Tip: in larger projects you would benefit from using protocols rather than a concrete type, but you can also often use closures with the coordinator pattern.

Now that our first view controller has a coordinator property, we need to modify the start() method of MainCoordinator to set that property:

vc.coordinator = self

That will allow ViewController to notify MainCoordinator when something interesting has happened. In this instance, that means we can take navigation away from our view controllers and make it the responsibility of our coordinator instead.

Open ViewController.swift, then cut to your clipboard all the navigation code from didSelectRowAt. That’s this code:

guard let detailVC = storyboard?.instantiateViewController(withIdentifier: "DetailViewController") as? DetailViewController else {
    return
}

detailVC.project = project
navigationController?.pushViewController(detailVC, animated: true)

Now go to MainCoordinator.swift and add this method:

func show(_ project: Project) {
    // paste your clipboard here
}

You’ll get a compiler error because it doesn't know what the storyboard property is, but that’s why we added the Storyboarded protocol earlier. We can bring the whole show() method down to this:

func show(_ project: Project) {
    let detailVC = DetailViewController.instantiate()
    detailVC.project = project
    navigationController.pushViewController(detailVC, animated: true)
}

Notice that I removed the optionality from navigationController – our coordinator always has one of these.

That method isn’t quite done yet, because when we create the detail view controller we need to inject our coordinator there too. So, add this line before the call to pushViewController():

detailVC.coordinator = self

Back in the ViewController class we can now ask the coordinator to take action on our behalf:

override func tableView(_ tableView: UITableView, didSelectRowAt indexPath: IndexPath) {
    let project = dataSource.project(at: indexPath.row)
    coordinator?.show(project)
}

That’s much cleaner than what we had before!

We can do something similar in DetailViewController. Go to its readProject() method and cut all the code inside there to your clipboard. Now open MainCoordinator.swift and paste it into this method:

func read(_ project: Project) {
    // paste your clipboard here
}

Again, we need to use instantiate() rather than the storyboard, and remove navigation controller optionality:

func read(_ project: Project) {
    let readVC = ReadViewController.instantiate()
    readVC.project = project
    navigationController.pushViewController(readVC, animated: true)
}

Back in DetailViewController we just need this one line inside the readProject() method:

coordinator?.read(project)

Cleaning up

If you run the app now it should look and work identically, but if we look at the code for the app you'll see our view controllers are now tiny. The code hasn’t been removed, just factored out so that it’s more flexible and reusable.

Yes, this is a huge step forward for this project, but it's just the beginning: there’s so much more we could do to make this code better.

For example, should ProjectDataSource both store our model data and act as a table view data source? Probably not – you could carve off the model handling code into a separate sub-object.

Or does injecting our coordinators create too much coupling for your liking? If so, we could go down a closure route instead, so that we could replace coordinators entirely if we wanted. To make that work, you’d add this property to ViewController:

var showProjectAction: ((Project) -> Void)?

Then in didSelectRowAt we could call that closure directly:

showProjectAction?(project)

All our coordinator code could stay, but we would need to update the start() method to this:

vc.showProjectAction = show

We could then do the same for the detail view controller, and boom – no more coordinator injection, and you can replace them with something else entirely that provides the required closure if you wanted to.

One particular pain point you might have noticed is that our NavigationDelegate mixes its own logic – “is this site allowed?” – with WebKit’s delegate method, decidePolicyFor. Any code where you blend your logic with Apple’s delegate methods becomes harder to test, so a smart idea would be to split up that code even further.

More specifically, we could rewrite it to this:

func isAllowed(url: URL?) -> Bool {
    guard let host = url?.host else {
        return false
    }

    if allowedSites.contains(where: host.contains) {
        return true
    }

    return false
}

And then put this inside decidePolicyFor:

if isAllowed(url: navigationAction.request.url) {
    decisionHandler(.allow)
} else {
    decisionHandler(.cancel)
}

With that changed we can start to write the first of many meaningful tests:

func testNavigationDelegateAllowsGoodSite() {
    let sut = NavigationDelegate()
    let url = URL(string: "https://www.hackingwithswift.com")
    XCTAssertTrue(sut.isAllowed(url: url))
}

func testNavigationDelegateDisallowsBadSite() {
    let sut = NavigationDelegate()
    let url = URL(string: "https://www.google.com")
    XCTAssertFalse(sut.isAllowed(url: url))
}

Those tests should both pass. Remember, changing code without having tests in place is rewriting not refactoring – I'm hoping your own projects will have lots more tests in place already!

Last but not least, with all this rewriting we introduced an important bug. Take a look at this code:

view = DetailView(project: project, readAction: readProject)

That creates a DetailView instance, and sets its readAction property to the readProject() method of our detail view controller. This creates a strong reference cycle: our view controller owns its view, and now the view owns its view controller.

To fix these kinds of problems, you should usually create a new closure with weak capturing of self, like this:

view = DetailView(project: project) { [weak self] in
    self?.readProject()
}

Where next?

For more advanced usages of coordinators and closures, you should read my article “using closures with the coordinator pattern”. You would also enjoy Soroush Khanlou's talk at NSSpain, Presenting Coordinators.

Sponsor Hacking with Swift and reach the world's largest Swift community!