Taming Hipmunk's iOS Code ✈︎

Hipmunk has been a startup for six years. In that time, we have developed an iOS app that not only searches flights and hotels, but can also be used to discover new destinations. The app has been written by a rotating cast of about 8 developers over 5 years, no more than 4 at a time, with skill levels from beginner to expert. Most classes were twisty mazes of fragile state with ad hoc dependencies, which made it difficult to ship new features quickly.

This is the story of how we were able to take an existing a codebase that had a lot of structural issues and not only address some of the technical debt, but also consistently deliver new features with a very small team.

Here are some interesting characteristics of the Hipmunk app:

  • A year ago, it contained about 59,000 lines of Obj-C and about 7,000 lines of Swift. Today, it contains about 46,000 lines of Obj-C and about 41,000 lines of Swift.
  • A year ago, it had 71 view controllers. Today it has 93.

I'll cover our changes in terms of four principles we focused on: testability, small files, predictability, and safe refactoring.

Testability

We'd like to be able to write unit tests for most classes without creating concrete instances of their dependencies. There were two areas that needed improvement to make such tests possible.

Removing global singletons

The old code used singletons extensively. If there was a FooDataSource, you would access it via [FooDataSource sharedFooDataSource]. While this was convenient at the call site, it meant that if you wanted to test the class that used FooDataSource, you had to replace the global instance during test setup. It's technically doable, but it requires test code in the implementation.

To avoid the singleton problem, we started using the dependency injection pattern. Classes store all their dependencies as properties, and their dependencies are passed (injected) either via the constructor or a hip_inject(dependencies...) method. That way, we can easily construct instances of objects with mocked dependencies.

To reduce boilerplate, we wrote the HIPInstanceLocator library, so you can define a class's dependencies just once and then inject them consistently at any point.

Older code still uses singletons today, and mostly still doesn't have unit tests. We expose singletons to dependency injected classes via HIPInstanceLocator.

Protocol dependencies instead of class dependencies

The old code referred to all types by their implementation classes. That means if you wanted to test a class that uses FooDataSource, you had to either replace the Obj-C runtime's FooDataSource class, or create an actual instance of it. These classes became difficult to test, which resulted in tests not being written for these classes or any class that depend on them.

In new code, to avoid these problems, we use protocol oriented programming. As much as possible, we define dependencies as protocols, and can therefore use either concrete implementations or mocks without doing any crazy magic. (To learn more, watch this WWDC 2015 talk.)

The old code has been refactored in some places, left alone in others.

Small files

If any file can accumulate a potentially infinite number of functions, we should find a way to split it up. It's okay for a namespace to be large, but if a file is large, things like private variables or related code can be difficult to follow.

Another benefit of small files is that you can quickly navigate using Xcode's "Open Quickly" (Cmd+Shift+O) feature. Instead of "open the file, then hunt for the method," you just open the file and can quickly skim the whole thing. And if most features have similar sets of classes, you can often guess the file's name on the first try without having been the one to write the feature.

This might sound like an oddly specific, trivial thing to put effort toward. But it was having a material effect on our daily productivity! Specifically, we have one class that contains methods for dozens of API requests for many features. It's 1,800 lines long and contains 77 functions. It is difficult to navigate a file this large, much less refactor.

To avoid adding additional complexity in this class, we started creating separate service classes for related sets of endpoints. This splits up the code into manageable chunks that are scoped to a specific endpoint or feature. Each service depends on an HTTP client object specified as a protocol, so we can easily test all code paths without running a local server.

Here's an example of a service that communicates with a todo list server, and the HTTP client protocol it depends on:

protocol HTTPClient {
    // we could implement this using NSURLSession, or a mock, or whatever
    func get(url url: NSURL, completionHandler: (AnyObject?, ErrorType?) -> ())
}

protocol TodosServicing {
    func get(callback: (TodoResponse?, ErrorType?) -> ())
    func upsert(item: TodoItem, callback: (ErrorType?) -> ())
}

class TodosService: TodosServicing {
    private let httpClient: HTTPClient

    init(httpClient: HTTPClient) {
        self.httpClient = httpClient
    }

    func get(callback: (TodoResponse?, ErrorType?) -> ()) {
        httpClient.get(url: "/todos/all") {
            json, error in
            if let apiResponse = json as? [String: AnyObject],
               let todoResponse = TodoResponse(apiResponse: apiResponse) {
               callback(todoResponse, nil)
            } else {
              // this is crappy error handling code!
              callback(nil, error ?? NSError(domain: "unknown", code: 0, userInfo: nil)
            }
        }
    }

    func upsert(item: TodoItem, callback: (TodoResponse?, ErrorType?) -> ()) {
        httpClient.post(url: "/todos/upsert", args: ...) {
            ...
        }
    }
}

We have been slowly removing methods from our giant API file. It's still pretty large, but our API code in general is much more manageable.

Predictability

The old code did not make consistent use of any design patterns. There were "data source" objects with somewhat clear responsibilities, but like so much Cocoa code, a huge amount of state was stored in view controllers. If we had to make changes to an existing feature, we'd have to first dig into the code to understand how its classes fit together.

When doing day-to-day feature development, there shouldn't be a need to invent basic patterns from scratch every time. Most features fetch some data from the internet, store it locally somewhere, and show it to the user in one or more views. If we can agree on a general pattern for implementing these features, we should be able to deliver them more consistently.

Model-View-ViewModel

We chose to use the Model-View-ViewModel (MVVM) pattern for all new feature development. There's a lot of literature about this pattern, but I'll go through a complete example using Swift and our observables library HIPEventedProperty.

Our old code uses MVVM classes if we've had to touch it within the past year.

Model

A model is the single source of truth for some data set. It loads the data (all at once or on demand) using one or more services, then provides it to other objects. In the Hipmunk app specifically, it typically exposes observables that represent the data, so other objects can subscribe and update themselves appropriately.

Models depend only on services and other models. Models do not use UIKit data types if possible. They only know about the data they are responsible for and nothing else.

Here's a model that represents the todos fetched by TodosService:

protocol TodosModeling {
    var todos: HIPEventedPropertyBasic<[TodoItem]> { get }
    func refresh()
    func upsert(item: TodoItem)
}

// This example just stores the cached data in memory, but you might want to use
// a local database like sqlite, Realm, or Core Data.
class TodosModel: TodosModeling {
    private let todosService: TodosServicing

    // See HIPEventedProperty documentation for specifics, but basically,
    // this lets you do two things:
    //   todos.value is the "current value" (array of TodoItems)
    //   todos.subscribe(..., callback: { /* called when value changes */ })
    let todos = HIPEventedPropertyBasic<[TodoItem]>([])

    init(todosService: TodosServicing) {
        self.todosService = todosService
    }

    func refresh() {
        todosService.get({ [weak self] in
            self?.todos.value = $0.items
        })
    }

    func upsert(item: TodoItem) {
      // There are some pretty horrific race conditions in this implementation.
      // Look elsewhere for examples of good async code.
      todosService.upsert(item, callback: { _ in /* ignore errors */ })
    }
}

View Model

A view model is the single source of truth for a user-facing subset of data in a model. It combines user input with models to expose a convenient set of properties that can be used to display something relevant to one or more specific views. It can also represent the state of a view controller in some UIKit-agnostic way. For example, it might know which filters have been applied to a search, what the search results are, and whether any particular search result is currently selected.

View models exist for two important reasons. The first is that view controllers tend to get too large, and we need a good place for logic that can be cleanly encapsulated outside views. The second is that multiple views often need the same subset of data and control over what that subset is. To continue our search example, one view controller might edit the search via a form, and the other might display the results. Both need access to the terms of the search.

Here's a view model that presents a subset of todos depending on a user-specified filter:

enum TodoListFilter {
    case All
    case DueToday
}

class TodoListViewModel {
    private var todosModel: TodosModeling

    let filter = HIPEventedPropertyBasic<TodoListFilter>(.All)
    let todos = HIPEventedPropertyBasic<[TodoItem]>([])

    init(todosModel: TodosModeling) {
        self.todosModel = todosModel

        // whenever the source data or the filter changes, recompute the
        // active list of todos
        let callback: () -> () =  { [weak self] in self?._updateFromModel() }
        _ = todosModel.todos.subscribe(withObject: self, callback: callback)
        _ = filter.subscribe(withObject: self, callback: callback)
    }

    private func _updateFromModel() {
        todos.value = todosModel.todos.value.filter({
            switch filter.value {
            case .All:
                return true
            case .DueToday:
                return NSCalendar(calendarIdentifier: NSCalendarIdentifierGregorian)!
                  .isDate($0.dueDate, inSameDayAsDate: NSDate())
            }
        })
    }
}

You might notice that we don't bother specifying a protocol for view models. That's because the models they depend on are really easy to mock, so mocking the view model is usually overkill.

Views and View Controllers

When starting out with UIKit, it can look like view controllers are the "controllers" in Model-View-Controller architecture. But they are view controllers. They control views. And that's all they should do. View controllers are part of the view.

View controllers take data from models and view models and put it into UIKit. Any given line of code in a view controller is either listening to a [view] model, doing something with UIKit, or passing control flow to another object.

Here's a view controller that displays the list of todos presented by TodoListViewModel:

class TodoListViewController: UIViewController {

    let refreshControl = UIRefreshControl()
    var todoListViewModel: TodoListViewModel!

    // "All" / "Due Today" segmented control in the navigation bar
    @IBOutlet var tabControl: UISegmentedControl!

    // Table view filling self.view
    @IBOutlet var tableView: UITableView!

    // Called by router after instantiation
    func inject(todoListViewModel todoListViewModel: TodoListViewModel) {
        self.todoListViewModel = todoListViewModel

        let callback: () -> () = { [weak self] in self?._applyViewModel() }
        _ = todoListViewModel.todos.subscribe(withObject: self, callback: callback)
        _ = todoListViewModel.filter.subscribe(withObject: self, callback: callback)
    }

    override func viewDidLoad() {
        super.viewDidLoad()
        refreshControl.addTarget(
            self, action: #selector(refresh(_:)), forControlEvents: .ValueChanged)
        tableView.addSubview(refreshControl)
    }

    override func viewWillAppear(animated: Bool) {
        super.viewWillAppear(animated)

        // Load todos if they haven't been loaded yet
        if todoListViewModel.todos.value.count < 1 { todoListViewModel.refresh() }

        _applyViewModel()
    }

    private func _applyViewModel() {
        // this loading state might be buggy, but this is just an example
        if refreshControl.refreshing { refreshControl.endRefreshing() }

        tableView.reloadData()
        tabControl.selectedSegmentIndex =
            todoListViewModel.filter.value == .All ? 0 : 1
    }
}


// MARK: Actions
extension TodoListViewController {
    // Called by the pull-to-refresh control
    @IBAction func refresh(sender: AnyObject?) {
        todoListViewModel.refresh()
    }

    // Called when the segmented control's value changes
    @IBAction func tabChangedAction(sender: AnyObject?) {
        todoListViewModel.filter.value =
            tabControl.selectedSegmentIndex == 0 ? .All : .DueToday
    }
}


// MARK: UITableViewDataSource
extension TodoListViewController: UITableViewDataSource {
    func numberOfSectionsInTableView(tableView: UITableView) -> Int {
        return 1
    }

    func tableView(
        tableView: UITableView,
        numberOfRowsInSection section: Int)
        -> Int
    {
        return todoListViewModel.todos.value.count
    }

    func tableView(
        tableView: UITableView,
        cellForRowAtIndexPath indexPath: NSIndexPath)
        -> UITableViewCell
    {
        let cell = tableView
            .dequeueReusableCellWithIdentifier("TodoCell", forIndexPath: indexPath)
        cell.textLabel?.text =
            todoListViewModel.todos.value[indexPath.row].text
        cell.detailTextLabel?.text =
            "\(todoListViewModel.todos.value[indexPath.row].dueDate)"
        return cell
    }
}

Safe Refactoring

We regularly redesign screens of the app and test two versions simultaneously. Any given view controller may be presented from multiple places in the app. Additionally, view controllers' APIs change over time; they gain new capabilities and new dependencies.

The old code used two anti-patterns that made refactoring view controllers difficult.

Configuration

Imagine you have a TodoViewController with public properties todos and backgroundColor and a dependency on TodosModel. Hipmunk's old code would present it this way:

class MyViewController: UIViewController {
    @IBAction openModal(sender: AnyObject?) {
        let vc = TodoViewController()
        vc.todos = self.todos
        vc.backgroundColor = UIColor.blueColor()
        vc.todosModel = TodosModel.sharedTodosModel()
        self.presentViewController(vc, animated: true, completion: nil)
    }
}

Now, what happens if TodoViewController gains a dependency on ContactsModel so you can start sharing todos with contacts? You'll have to search the whole codebase, find every instance where TodoViewController is configured, and update it. Then, hopefully, test each instance by hand or using your automated UI tests. If you missed one, your app will probably crash.

This problem can be avoided if every view controller uses methods for configuration and dependency injection:

class MyViewController: UIViewController {
    @IBAction openModal(sender: AnyObject?) {
        let vc = TodoViewController()
        vc.injectDependencies(todosModel: self.todosModel)
        vc.configure(todos: todos, backgroundColor: UIColor.blueColor())
        self.presentViewController(vc, animated: true, completion: nil)
    }
}

Now, if you change the configuration or dependencies of TodoViewController, the app won't even compile until all the call sites have been fixed.

Instantiation and presentation

In the example above, MyViewController presents TodoViewController. Now, what if we've redesigned TodoViewController and have a new variant, NewTodoViewController? We'll have to search the whole codebase again and put if/else statements around all TodoViewController presentations. Gross!

To solve this problem, we introduced a Router class.

Router

The Router (there's only one) is responsible for configuring, presenting, and transitioning between view controllers. To avoid unbounded linear growth, it is divided into multiple feature-specific files using class extensions:

HIPRouter.swift
HIPRouter+Flights.swift
HIPRouter+Hotels.swift
...

The router owns a ‘HIPInstanceLocator’ that it uses to initialize view controllers and their dependencies.

To continue our todo example above, the router method would look like this:

extension HIPRouter {  // todo list extensions
    func showTodoList(
        presentingViewController: UIViewController,
        todos: [Todo],
        backgroundColor: UIColor)
    {
        if self.featureFlagModel.isFlagEnabled(.NewTodoList) {
            let vc = TodoViewController()
            self.instanceLocator.applyInjector(vc)
            vc.configure(todos: todos, backgroundColor: backgroundColor)
            presentingViewController
                .presentViewController(vc, animated: true, completion: nil)
        } else {
            let vc = NewTodoViewController()
            self.instanceLocator.applyInjector(vc)
            vc.configure(todos: todos, backgroundColor: backgroundColor)
            presentingViewController
                .presentViewController(vc, animated: true, completion: nil)
        }
    }
}

class MyViewController: UIViewController {
    var router: HIPRouter!  // view controllers usually depend on the router

    @IBAction openModal(sender: AnyObject?) {
        router.showTodoList(
            presentingViewController: self,
            todos: self.todos,
            backgroundColor: UIColor.blueColor())
    }
}

We don't use the router for absolutely all view controller work. A lot of the time, it makes more sense to keep related view controllers in a storyboard and use segues to transition between them. HIPInstanceLocator makes sure each view controller has its dependencies injected. But we use the router for anything with complex configuration or multiple use cases.

We haven't had to touch the old code very much to make the router work, but we have made sure all important view controllers can be instantiated by the router, old or new. The pattern is agnostic to the architecture of each view controller, since each method in the router can do whatever setup that particular view controller needs.

Parting thoughts

When diving into a pile of unfamiliar code, it's really easy to say, "This is too awful. We can't fix it. We have to rewrite it or leave it in its crappy state." But in our experience, you can keep developing new features while paying down technical debt, and end up in a really good place. There are still a lot of untamed monsters in the Hipmunk app, but we are clearly doing better because of the efforts we've made to clean up over time.