Let's Play: Refactor the Mega Controller!

So you’ve got a huge view controller that knows about everything. It’s become a puppeteer mastermind whose responsibilities have somehow grown to simultaneously encompass disk I/O and navigation bar styling. Andy Matuschak live codes solutions to reduce the size of the beast, and refactors out its responsibilities without breaking the world. Let’s play!

---

Introduction

I’m Andy Matuschak, and today I’ll demonstrate refactoring a really bad view controller. I lead mobile engineering at Khan Academy, where we’re trying to create a free, world-class education for everyone, everywhere. Before that, I spent many years working on UIKit at Apple, so I feel I’ve earned the right to make fun of UIViewController.

The original mega view controller and refactoring steps are available here on GitHub, so use it to follow along.

The Awful View Controller

I created a mock app called the MegaController, and it’s a to-do list. You can add tasks, then delete them to check them off. It categorizes different tasks by how long you have to complete it under headers. The color of the top navigation bar is a measure of how screwed you are; if you should be busy the color of the bar is red (“Dangerous”), otherwise it’s orange (“Warning”) or yellow (“Placid”). This is a pretty simple app.

This view controller also serves as four things, which is way too many to be responsible for:

class ViewController: UITableViewController, NSFetchedResultsControllerDelegate, UIViewControllerTransitionDelegate, UITViewControllerAnimatedTransitioning

Take a look at the awful view controller class. Here’s what this class is doing so far:

• FetchedResultsController
• Core Data stack
• Configures predicate and fetch request
• Performs I/O
• Visuals and presentation
• Significant transformation for presentation
• TableViewDelegate
• TableViewDataSource
• Knows titles of sections
• Hard coupled to cell presentation
• Handling dynamic changes to fetched results
• Custom presentation animation for another view controller
• Heinous unwind coupling

Let’s look at some of these in detail:

FetchedResultsController

It has a FetchedResultsController, which it’s configuring and it’s the delegate of. It’s reading from the file system, and is starting all the Core Data stuff. This is the Core Data template. It starts up the Core Data stack in this view controller.

ViewDidLoad

In viewDidLoad(), we configure a pretty complicated predicate and fetch request. We configure FetchedResultsController, and then we have it perform a fetch. We perform IO. Then we’re doing some kind of clever caching thing that is not yet clear. We’re updating the navigation bar and we’re updating the status bar, so it’s also responsible for visuals and presentation. It also contains the code that sorts these results into sections dynamically, depending on the date. It defines the “soon” and “upcoming” sections using arbitrarily picked numbers. There’s significant transformation for presentation happening. It’s a table view delegate and a table view data source, and it knows the titles and the strings of the sections. It knows how to make that association between zero and now.

Table View

//cellForRowAtIndexPath
switch numberOfCalendarDaysUntilTaskDueDate {
  case 0:
    description = "Today"
  case 1:
    description = "Tomorrow"
  default:
    description = "In \(numberOfCalendarDaysUntilTaskDueDate) days"
}

cell.detailTextLabel.text = description.lowerCaseString
return cell

This is what is responsible for this string on the side which says, “Today”. There’s also a bug: a task due yesterday is supposed to say, “Yesterday”, but there’s no code to do that, so it just says “In negative one days”. This code is also hard-coupled to the cell presentation. We’re reaching into the cell and setting the text property on the detail text label. We’re also setting the text property on the cell’s text label. If we wanted to change the way that the cell presented its information, we would also have to change this, so that’s yet another responsibility we should consider refactoring out.

Handling Dynamic Changes

Yet another responsibility it shouldn’t be handling is dynamic changes to the fetched results.

func controller(controller: NSFetchedResultsController, didChangeObject anObject: NSManagedObject, atIndexPath indexPath: NSIndexPath?, forChangeType type: NSFetchedResultsChangeType, newIndexPath: NSIndexPath? ) {
  switch type {
    case .Insert:
      //insert task to table view
    case .Delete:
      //remove from table view
  }
}

FetchedResultsController emits an inserted object or tells you that it’s deleted an object, and you go and tell the table view in turn. It’s made significantly more complex, in this case, because of these dynamic sections. The FetchedResultsController isn’t capable of computing these sections, because it would have to be a stored property on the managed object. It’s taking the index path that the FetchedResultsController knows about, and it’s computing the presentation index path within the “soon” or “upcoming” section that the purportedly deleted or inserted cell is supposed to go.

Navigation Bar and Add Task

The navigation bar and the preferredStatusBarStyle are also both part of visuals and presentation. Also, there’s some segue stuff, because when we press “plus” there’s a custom animation. So, a custom presentation animation for another view controller. This is a view controller that has hard-coupled knowledge of how another view controller is supposed to be presented.

Heinous Unwind Coupling

The last one is unwindFromAddController. This one’s particularly heinous. This is an ID action that’s been built from ID. We’re pulling the source feed controller out of the segue, and we’re just force casting it through an AddViewController. That’s the hard-coupled part of this relationship. Furthermore, we’re pulling data directly out of the UI lens of that view controller. I’m sure you have code like this in your application. We’re also directly modifying the persistence layer. Thus, this one method is a model, view, and controller all in one.

Let’s Play: Fix Them

Let’s try to tackle these and write tests as we go. Some people believe in mocking and stubbing everything, and automating every action that could possibly take place with this view controller. I think that’s interesting, but I’m much more interested in testing the actual logic that’s taking place here, so I’m going to focus on testing that. I’m not going to test anything that feels like XML to me. View controllers aren’t XML. But sometimes, maybe not in this view controller, there’s a method in the view controller where there isn’t computation.

Navigation Bar Theming

I can come up with an interesting Swift approach to refactoring the navigation and status bar styling and theming out of the view controller. NavigationTheme in our new enum is going to be either Placid, Warning, or Danger.

enum NavigationTheme {
  case Placid
  case Warning
  case Danger

  var barTintColor: UIColor? {
    switch self {
      case .Placid: return nil
      case .Warning return UIColor(red: 235/255, green: 156/255, blue: 77/255, alpha: 1.0)
      case .Danger return UIColor(red: 248/255, green: 73/255, blue: 68/255, alpha: 1.0)
    }
  }
}

In Objective-C, this would just be numbers, but here we’re allowed to do something much fancier: we’re allowed to make this navigation theme have, for instance, a bar tint color. This is now a method that is pure, and it’s pulled out of everything. There is an interesting piece of the logic that goes in the navigation theme, and that’s really how we know whether we’re in a placid scenario or in a dangerous scenario. I’m going to write that as an extension, because I want to emphasize the fact that this could be in a totally different file.

extension NavigationTheme {
  init(forNumberOfUpcomingTasks numberOfUpcomingTasks: Int) {
    switch numberOfUpcomingTasks {
      case 0...3:
        self = .Placid
      case 4...9:
        self = .Warning
      default:
        self = .Danger
    }
  }
}

If we were writing Java, then this might be a factory, but we’re not writing Java. Self is Placid, and so I’m writing this in a more idiomatically Swift-y style, which is a custom constructor. I’m writing it deliberately in this extension to emphasize the fact that the implementation of the type itself doesn’t have to know anything about this constructor. I’m also writing this constructor in such a way that if there are any private details about this navigation theme type, I would try my darnedest not to consult them in this constructor. This should be able to be an external type, if it wanted to be a navigation theme factory, but we don’t do that in Swift.

I haven’t implemented the other styling things yet, and that’s deliberate, because I want to write a test. We’re already at a point where we can write a meaningful test: testNoTaskIsPlacid. I’m using Xcode’s unit testing framework here, and I’m using the lovely thing that is new in Swift 2, where I can test internal stuff from tests by using @testable. Note that the navigation theme is internal. Normally from another module I wouldn’t be able to reference it, but because of @testable I can. Now I can assert that if I try to make a navigation theme for zero tasks, I’m going to get a placid navigation theme.

//NavigationThemeTests.swift

@testable import MegaController
import XCTest

class   NavigationThemeTests: XCTestCase {
  func testNoTaskIsPlacid() {
    XCTAssertEqual(NavigationTheme(forNumberOfUpcomingTasks: 0), .Placid)
  }
}

I can also test that some tasks are warning-inducing. I can also test that too many tasks is dangerous, as we all know very well. Our tests pass, so now I can go back to my navigation theme with the confidence that this is the interesting domain knowledge that’s being captured here.

My designer gave me a spec that said, “In this situation you’re supposed to be placid,” and that’s what I’ve implemented here. My tests now are testing the implementation of that domain logic. I’m going to implement the rest of these guys in the NavigationTheme. titleTextAttributes is a really lovely type that comes from Objective-C. When we’re placid, we don’t have any special title text attributes. When we’re in warning or danger territory, we’re going to have white text, and similarly, we’re going to have a tint color sometimes.

enum NavigationTheme {
  case Placid
  case Warning
  case Danger

  var barTintColor: UIColor? {
    switch self {
      case .Placid: return nil
      case .Warning return UIColor(red: 235/255, green: 156/255, blue: 77/255, alpha: 1.0)
      case .Danger return UIColor(red: 248/255, green: 73/255, blue: 68/255, alpha: 1.0)
    }
  }

  var titleTextAttributes: [String: NSObject]? {
    switch self {
      case .Placid: return nil
      case .Warning, .Danger:
        return [NSForegroundColorAttributeName: UIColor.whiteColor()]
    }
  }

  var tintColor: UIColor? {
    switch self {
      case .Placid: return nil
      case .Warning, .Danger:
        return UIColor.whiteColor()
    }
  }

  var statusBarStyle: UIStatusBarStyle {
    switch self {
      case .Placid: return .Default
      case .Warning, .Danger: return .LightContent
    }
  }
}

There’s no tint color when the bar is Placid. We’re going to use the system’s default blue for that plus button in the upper right, but when we’re in warning or in danger, we want to use white. The preferred status bar style is going to be a little interesting to route through. When we’re Placid, we’re going to have the system default style, and when we’re in Danger territory, we’re going to have light content.

We have again this sort of XML-like description of this navigation theme, and I would like to consequently get rid of all of this code that is in my view controller. I’m trading, moving it around, and more importantly moving it to a place that’s isolated from everything else. This file doesn’t care about anything else in my program, and nothing is going to run in this file unless I ask it to run. Now, this is very unlike a view controller, where code in your view controller will run just because the status bar time changed. That is not true of the navigation theme, and that’s why I like things that are in this style — I can understand this file and wrap my head around it.

I have to get this gruesome stuff out of the view controller. This view controller has no business knowing that it’s in a navigation controller. This view controller is reaching up to its parent navigation controller; there could be a container view controller around this thing; who knows what view controller that is going to be, and it’s changing the navigation bar color. If some other view controller also wants to change the navigation bar color then you’re completely screwed, so this was awful in the first place.

Status Bar Style

There’s also preferred status bar style here, which is kind of interesting because navigation controllers don’t normally ask their children for a preferred status bar style. I already had to override the navigation controller to even get it to do that.

//NavigationController.swift

class NavigationViewController: UINavigationViewController {
  override func childViewControllerForStatusBarStyle() ->
  UIViewController? {
    return topViewController
  }
}

To remove inadvisable behavior, we’re going to operate on a principle that I spoke about at WWDC a couple of years ago, where I tried to ask myself the following questions: “Where is truth? Who is the ultimate responsible entity for what the color of that navigation bar should be? Who should know that?” I propose that it’s the parent of both of these things. The parent of both of the things makes sense because that’s the way to avoid either of these things knowing about each other. There’s no particular reason why this navigation controller should know the identity of the top view controller within it. The navigation controller class shouldn’t be hard-coupled to my view controller class, and certainly not vice versa. So that means that we need some kind of outside arbitrator, which the default would be the app delegate. That seems like a terrible idea, but I’ll do my best to make it not a terrible idea, and then we’ll judge it together.

In order to achieve this outside entity knowing what truth is, I need to be able to send events to it in a decoupled way. I’m going to do that in a really dumb way: themeDidChangeHandler

var themeDidChangeHandler: ((NavigationTheme) -> Void)?
var theme: NavigationTheme {
    return NavigationTheme(forNumberOfUpcomingTasks: fetchedResultsController?.fertchedObjects?.count ?? 0)
}

I’m going to call this thing whenever the navigation theme changes. Furthermore, I’m also going to have an accessor for the current theme, navigation theme, which is pretty easy to compute because all I have to do is compute the navigation theme for the number of upcoming tasks, where the number of upcoming tasks is the FetchedResultsController's fetched objects count, or zero if that isn’t around, because this is optional. In update navigation bar, what I’m going to do instead is call the themeDidChangeHandler with my theme.

//ViewController.swift
...

func updateNavigationBar() {
  themeDidChangeHandler?(themne)
  ...
}
...

This thing doesn’t know about the navigation controller that it’s in anymore, and also doesn’t know how to change the navigation bar of anything. One thing that I like about storyboards is that they decouple one view controller from knowing about another in simple cases. One thing that I want to show is that clicking this plus button here presents the AddViewController, and yet there is no method here. There’s no method that’s presentAddViewController. That knowledge of the identity of the view controller that is to be presented when the plus button is pressed is encoded in the storyboard, which is a way of decoupling. However, I would like to be able to refer to these view controller instances, and you can’t. This is just going to be a hack for now. I’m going to make a few assumptions here, such as that the navigation controller is the root view controller and that we have a window.

//AppDelegate.swift
class AppDelegate: UIResponder, UIApplicationDelegate {
  var window: UIWindow?

  private var navigationController: NavigationController {
    return window!.rootViewController! as NavigationController
  }

  private var storyboard: UIStoryboard {
    return UIStoryboard(name: "Main", bundle: nil)
  }

  private lazy var viewController: ViewController = {
    return self.storyboard.instantiateViewControllerWithIndentifier("Primary") as! ViewController
  }()

}

To finish the decoupling of the view controller instantiation from storyboards, we’ll finally create this themeDidChangeHandler that change the navigation bar color on launch.

//AppDelegate.swift
...
func application(application: UIApplication, didFinishLaunchingWithOptions launchOptions: [NSObject : AnyObject]?) -> Bool {
  viewController.themeDidChangeHandler = { [weak self] in
    if let navigationController = self?.navigationController {
      //avoid this
      navigationController.navigationBar.barTintColor = theme.barTintColor
    }
  }
  navigationController.viewControllers = [viewController]
  return true
}
...

So, when the theme changes, themeDidChangeHandler is going to be a piece of code. If we’re going to pull out the navigation controller, we have to do this dance because we’re weak. What I don’t want to do here is what you see above. navigationbar.barTintColor = theme.barTintColor is not pleasant. There’s going to be four of them, and it’s not going to be like XML anymore. In order to avoid that, I’m going to make a theme. My strategy here is simply decomposition. I’m going to make this thing that applies a theme to a navigation bar:

//UINavigationBar-Theme.swift

extension UINavigationBar {
  func applyTheme(theme: NavigationTheme) {
    barTintColor = theme.barTintColor
    tintColor = theme.tintColor
    titleTextAttributes = theme.titleTextAttributes
  }
}

//AppDelegate.swift
func application(application: UIApplication, didFinishLaunchingWithOptions launchOptions: [NSObject : AnyObject]?) -> Bool {
  viewController.themeDidChangeHandler = { [weak self] in
    if let navigationController = self?.navigationController {
      navigationController.navigationBar.applyTheme(theme)
    }
  }
  navigationController.viewControllers = [viewController]
  return true
}

This file is totally isolated from everything (except for all of UIKit). There’s nothing I can reasonably do about that until everybody starts adopting React Native and we all abandon UIKit completely. I expect that now what we’ll have is the correct bar colors and themes.

We got yellow, but we got the wrong status bar color. What we’re going to do is move this hack that makes the navigation controller know that its top view controller should be responsible for choosing what the status bar style is. The top view controller is also the one responsible for calling set me, the status bar appearance update, on its parent whenever it wants the status bar to have changed. We’re going to remove that and make it data-driven instead.

//NavigationController.swift

class NavigationViewController: UINavigationViewController {
  var statusBarStyle: UIStatusBarStyle = .Default  {
    didSet {setNeedsStausBarAppreanceUpdate()}
  }

  override func preferredStatusBarStyle -> UIStatusBarStyle {
    return statusBarStyle
  }
}

//AppDelegate.swift
...
func application(application: UIApplication, didFinishLaunchingWithOptions launchOptions: [NSObject : AnyObject]?) -> Bool {
  viewController.themeDidChangeHandler = { [weak self] in
    if let navigationController = self?.navigationController {
      navigationController.navigationBar.applyTheme(theme)
      navigationController.statusBarStyle = theme.statusBarStyle
    }
  }
  navigationController.viewControllers = [viewController]
  return true
}

What I like about what we just did is that truth is now clearly represented. Nobody is fighting over the status bar style of this navigation controller, and nobody’s fighting over the bar tint. Instead, we have straightforward flow of data and control, and we have it happening in a tree-like fashion, which is my favorite shape. I think that programs are very easy to understand when the control flow through them and the ownership through them and the data flow through them is all linear up and down trees. Now we have that for this one thing in this God-awful list of view controller things, so we can check that one off.

Separate The Data Source

Let’s try to do something about the data source situation here. I’m going to pull all of this stuff about core data out into another file, and I’m going to call it core data stack.

//CoreDataStack.swift

import CoreData
import Foundation

class CoreDataStack {
  lazy var applicationDocumentsDirectory: NSURL = {
    let urls = NSFileManager.defaultManager().URLsforDirectory(.DocumentDirectory, inDomains: .UserDomainMask)
    return urls[urls.count-1]
  }()

  lazy var managedObjectModel: NSManagedObjectModel = {
    let modelURK = NSBundle.mainBundle().URLForResource("MegaController", withExtension: "momd")!
    return NSManagedObjectModel(contentsOfURL: modelURL)!
  }()

  lazy var persistentStoreCoordinator: NSPersistentStoreCoordinator = {
    let coordinator = NSPersistentStoreCoordinator(managedObjectModel: self.managedObjectModel)
    let url = self.applicationDocumentsDirectory.URLByAppendingPathComponent("SingleViewCoreData.sqlite")
    do {
      try coordinator.addPersistentStoreWithType(NSSQLiteStoreType, configuration: nil, URL: url, options: nil)
    }catch{
      fatalError("Couldn't load database: \(error)")
    }

    return coordinator
  }()

  lazy var managedObjectContext: NSManagedObjectContext = {
    let coordinator = self.persistentStoreCoordinator
    var managedObjectContext = NSManagedObjectContext(concurrencyType: .MainQueueConcurrencyType)
    managedObjectContext.persistentStoreCoordinator = coordinator
    return managedObjectContext
  }()
}

I’m going to go ahead and say that only the managed object context is what clients get to access. If I were to try to test with this, I would have no way to do an in-memory store. We can do this incrementally, and that’s one of the things that I like about refactoring in this fashion, is that we can pull things out piece by piece, and every time we pull something out it gets better, and more understandable. After we pulled the core data stack out, and now we go through and fix stuff up in ViewController.

//ViewController.swift

...
  private let coreDataStack = CoreDataStack()
...
  fetchedResultsController = NSFetchedResultsController(fetchRequest: fetchRequest, managedObjectContext: coreDataStack.managedObjectContext, sectionNameKeyPath: nil, cacheName: nil)
...
//replace all managedObjectContext with coreDataStack.managedObjectContext

We don’t want to leave any of that in this file, but we’re not going to get all the way there. However, what I did was incremental improvement. It used to be that there were four variables available for anybody to screw with, and now there’s one. I can more tightly control where it’s initialized, and the next modification I would make if I wanted to continue down this path, would be that I would inject this into the class. That also means that I can remove the coupling of the instantiation from this class as well, which would be lovely because it would be very natural for me to want to create another view controller in this project and to give it a core data stack. At that point, having this code here in this view controller would be completely inappropriate.

We still, however, have a ton of core data goo in this file. I’ll take a moment to rant about core data, because I think the principle at stake here is going to be more useful than watching me perform this transformation. The goo is deep in core data. The goo is deep in core data not because of specific design decisions that people complain about with core data, such as “The API service is too big,” or “All these methods are really complicated or faulty.” At root, there’s shared mutable state everywhere in core data. If you have an NSManagedObject, that NSManagedObject has a reference to the context that created it and that manages it. It’s mutable because a managed object can change out from under you if anything else can possibly get a reference to that managed object, which transitively is anything else that can possibly get a reference to the managed context from whence that managed object came. Anything can happen if you’re not careful, but there’s no good way to be careful. To me, that’s the thing that’s completely maddening about core data and all ORMs, really.

My natural next step here would be to progressively remove all knowledge of core data from this view controller. I’ll outline quickly how I would do that I have some sketches of how I would do it. In this class, we could pull up the list, a FetchedResultsController, that’s a coupling with core data. The core data stack’s gone. The predicate and the fetch requests, so that’s a coupling to core data. The IO, the save, the save bits, those are in here. And also, handling the dynamic changes to the fetched results. I would just start pulling out those responsibilities one by one, and at the very bottom of all of that, what gets pulled out is the NSManagedObject itself, because the NSManagedObject creates dependencies to everything else it touches. I would keep pulling things out until what I had was just the data that was necessary for presentation. And when I wanted to create a task, I would only have to supply the data that’s necessary to create that task. This is how you create a core data object. You get the managed object context, persistent stores coordinators, managed object models, entity for the string task, and you insert it into a specific context.

let newTask = NSManagedObject(entity: coreDataStack.managedObjectContext.persistentStoreCoordinator!.managedObjectModel.entitiesByName["Task"]!, insertIntoManagedObjectContent: coreDataStack.managedObjectContext)
newTask.setValue(addViewController.textField.text, forKey "title")
newTask.setValue(addViewController.datePicker.text, forKey "dueDate")
try! coreDataStack.managedObjectContext.save()

I’ve made this worse than it had to be, you could have made a class with an NSManagedObject subclass for that. NSManagedObject subclass is coupled to all this stuff, but it’s just hiding it and it doesn’t even hide it very well.

I made this thing called a DataManager, and it still speaks in terms of managed objects but it does so decreasingly. Let’s quickly look at the state of the view controller after this data manager exists. The FetchedResultsController is gone, because it is in the data manager. And most of this stuff is starting to look pretty XML-y. I don’t love that this view controller is still a table view delegate and data source, but at least the implementations aren’t doing anything. They’re not encoding much knowledge. We have other methods that are friendlier versions of NSFetchResultsController’s something changed method, I just made them more specific. You’ll notice that these don’t deal with NSManagedObject at all. There is still a dependency on NSManagedObject because of updating the cell in cellForRowAtIndexPath.

There’s only one step left to remove this. The UpcomingTaskDataManager exposes two properties, and one of them is interesting. This is just a sum that I used for the theme. It exposes these task sections, which is a list of list of managed objects. And that task section’s array, it is updated as things come and go. This isn’t really storage: it’s not an array of array of array of managed objects. It’s actually coming from a results cache, which is a struct. The reason that structs are nice is that now this is one step away from being totally isolated from everything. The only way in which it’s not isolated is that it’s dependent on NSManagedObject.

We can look at the way in which it depends on NSManagedObject, specifically, what it needs from NSManagedObject. All it needs from NSManagedObject is to be able to pull a due date out. This could probably be a list of lists of things that respond to the due date property, and this class would still compile. At this point all this code becomes vastly easier to reason about because of the same reasons as the navigation theme refactoring that we did get through, namely, that code here is only going to execute when its direct parent tells it to execute, and it is immutable and fully isolated. I know that it seems weird for me to say that it’s immutable when there’s this word “mutating” here, but this is another interesting principle to know about. It’s still immutable. The variable slot which holds the instance is mutable, but the value itself is immutable. Thus I could serialize one of these caches to a file safely, or I can cons one up in a test, or I could send one over the network, et cetera. They suddenly become interchangeable.

All these things are really valuable, and it also makes tests really easy to write. I would make that transformation, I’d get the NSManagedObject out, I’d get the NSManagedObject out of this property, and then my view controller would be free from the tyranny of NSManagedObject and everything would be beautiful. We’d only have a couple of things left to do.

Decoupling the Table View and View Controller

I’ll speak briefly to how I would pull this part out, namely, configuring the cell, the coupling between the view controller and the table view cell. There is a design pattern called ViewModels that I think has almost got it right. So the way that that design pattern works is that you give your view a reference to this thing called a ViewModel that only has the information that the view needs in order to render itself. The ViewModels are also the things which implement the actions for the view. I don’t thing that’s a good idea because then they’re not inert, and control comes from multiple directions. If we take just the first part, we can make a thing that I call ViewData:

class MyCell {
  struct ViewData {
    let title: String
    let timeSummary: String
    init(task: Task) {

    }
  }

  var data: ViewData {
    didSet {
      //set values of views
    }
  }
}

I’m going to write an inline class here. We might write my cell, and my cell might have a ViewData that looks like it has a title. This is the data that it needs to render. It has a title, and it has like a time summary — that’s the thing that says “one day from now,” so these values are both completely inert. I’m not passing it a hot NSManagedObject. I’m not passing it anything that can do anything, I’m just passing it a struct of strings. I can move this knowledge out of here by making an initializer for my ViewData struct from whatever it is that starts coming out of my data manager. Right now it’s still a managed object, so I could initialize my ViewData from the managed object, or, if it started to be some value-type task, then I could initialize it from that. Now this initializer would encode the dependency on my special relative DateTime formatter, which I made in this sketch but didn’t make with you all. It does what you’d expect, where it figures out what the string should be. Again, this is totally isolated.

That’s what extracting this logic would look like. The cell would have a setter for its ViewData, and all didSet has to do is update the values of the views. The view controller doesn’t have to know about the views that are in the cell. I could change around the presentation of the cell without changing the view controller. I could change around my model entity, I could look at a field of task, or I could change the name of the field of new task without having to change the meat of the implementation.

---

Read the source code and learn from more refactoring steps by looking through the commit history of the repo on GitHub.

---

Q&A

Q: Do you have any opinion on saving structs to disk? NSKeyedArchiver needs NSCoding and NSCopying and all that.

Andy: I just wrote a thing that serializes a bookmark database. What I did is create a serializer struct and it is allowed to be a struct because it doesn’t do anything. It returns data and it takes in some bookmarks, but that’s all it does. It doesn’t even care what kind of thing is holding the bookmarks, because it doesn’t need to. It does property lists serialization, and it encodes them by mapping over the bookmarks. It’s what you’d expect.

Q: When you instantiate a controller from the storyboard, it hands you the instance, so it removes the ability to instantiate a root controller with its appendices to make sure that it’s properly initialized. Is there a better way?

Andy: Not that I know of, other than not using storyboards. What if you represent your entire application as an enum? That could work. Then, instead of having things follow the UINavigationController, you have the navigation controller follow the state. You just instantiate view controllers according to this thing. UIKit really fights you on it though, so proceed with caution.

Q: You mentioned React Native — what would you say is holding you back from adopting it as your main UI framework?

Andy: JavaScript is terrible, but it’s also probably the future, unfortunately. We were the first big project to adopt Swift, as far as I know; we wrote a few tens of thousands of lines of Swift, and we just completely broke the tools. We are not really even a tech company, Khan Academy is a tiny nonprofit with a couple engineers, and our problems are not technical problems. The fact that we’re breaking the tools is because we’re early adopters. I’m a little more conservative after that experience, and would like to see someone else be the guinea pig with React Native.

The other thing is that the Android support isn’t public yet. As a result, even if we wanted to adopt, it wouldn’t get as much benefit. If/when we do adopt (and I think it’s likely that we will), we will not be using plain JavaScript. We would be using Flow, we would have type tracking, and probably coroutines and a bunch of other things to make the experience more bearable.

Q: You refactored a lot out of the view controller. At what point do you stop or feel that you abstracted too much for others to follow?

Andy: That’s a huge problem. You know what we call the opposite of spaghetti code? Ravioli code. An interesting contrast that balances coupling is a term called cohesion. Coupling is that you don’t want to have too many disparate things in the same type. A concern of cohesion is that you don’t want to have too many things that are essentially the same concept spread over too many places.

There are great examples of really bad doc strings. I was about to tell you how we’re dealing with that these days, because we have been having trouble with it. We write a bunch of EGs in our doc string. We write, “This thing is used by this thing for this piece of UI,” and then the coupling is not really coupling, it’s documentation coupling. It’s like a map for whenever you’re looking at one component, you have some pointers to go and look at neighboring components, and you can usually feel your way around then. It’s a serious concern though, and it’s difficult to find that balance.