Return to Revisions

A couple of thoughts.

• The presence of init(viewModel:) in the view controller (with its associated comment about using this during testing) seems to suggest that you plan on testing the view controller. But one of the central tenets of MVP, MVVM and the like is that the goal is to have a UIKit-independent representation. Part of the reasons we do that is because that is the object that we’ll test, not the view controller.

Bottom line, I’d be inclined to retire init(viewModel:) from the view controller and restrict the testing to the view model.

For example, a MVP might look like:

enter image description here

Or, in MVVM, you’ll see structures like:

enter image description here

(Both images from Medium's iOS Architecture Patterns.)

But in both cases, the mediator (the view model or the presenter), should not be UIKit-specific, and it’s that mediator on which we’ll do our unit testing.

• The view controller has a method updateUI, which is adding the view to the hierarchy. I’d suggest you decouple the initial "configure the view" from the "view model has informed me of some model change".

• MVVM generally suggests that you’re doing some "data binding", where the initial configuration of the view controller sets up the connection between presenter events and UIKit control updates and vice versa. It’s hard to tell, but this feels like you’re got the beginning of something with a more MVP je ne sais quoi than MVVM. That’s not necessarily wrong, but they’re just different.

• In BreachViewModel is updating the text property of the UILabel called nameLabel within the view. To my two prior points, the view model, itself, should be UIKit dependent, and it definitely shouldn’t be reaching into a subview of the view and updating the text itself. If this was MVVM, you’d bind the label to the view model and have the update take place that way. If this was MVP, the presenter should just inform the view controller that the value has changed and the view controller would update the UIKit control.

But avoid having anything UIKit specific in the view model.

• A few observations in fetchData:

• You have paths of execution where you don’t call the completion handler. You generally always want to call the completion handler, reporting success with data or failure with error information. Perhaps adopt the Result<T, U> pattern you did with ClosureHTTPManager (like we did in the answer to your earlier question).

• You are decoding your JSON twice. Obviously, you only want to do that once.

• This is personally a matter of taste, but I’m not crazy about the view model doing JSON parsing. That seems like the job of some API layer, not the view model. I like to see the view model limited to taking parsed data, updating models, applying business rules, etc.

I think the aforementioned iOS Architecture Patterns is an interesting discussion of many of these issues. I also think Dave DeLong’s A Better MVC is an interesting read.

Generally, if you were going to write MVVM, you’d use a framework like Bond or SwiftRX to facilitate the bindings. But let’s contemplate what a minimalist implementation might look like. (In the absence of these binding networks, this is arguably more MVP than MVVM, but it illustrates the basic idea that this view-model/presenter is where the logic goes and the view controller is just responsible for hooking it up to the view.)

Bottom line, the view controller would basically set up the view model (possibly passing in initial model data), and tell the view model what it wanted to do when the view model wanted to inform us

class BreachViewController: UITableViewController {
 var viewModel = BreachViewModel(model: nil)
 var dimmingView: UIView?
 
 override func viewDidLoad() {
 super.viewDidLoad()
 
 // tell view model what you want it to do when the model changes
 
 viewModel.breachesDidChange = { [weak self] result in
 self?.tableView.reloadData()
 }
 
 // tell view model that you want it to fetch from the server
 
 viewModel.fetchBreaches() { [weak self] result in
 if case .failure(let error) = result {
 self?.showError(error)
 }
 }
 }
}

The view model could have a method to perform the request and inform the view (controller) of the changes:

class BreachViewModel {
 
 var breaches: [Breach]? {
 didSet { breachesDidChange?(breaches) }
 }
 
 var breachesDidChange: (([Breach]?) -> Void)?
 
 init(model: [Breach]?) {
 breaches = model
 }
 
 func fetchBreaches(completion: @escaping (Result<[Breach], Error>) -> Void) {
 ApiManager.shared.fetchBreaches { [weak self] result in
 guard let self = self else { return }
 
 switch result {
 case .failure:
 self.breaches = nil
 
 case .success(let breaches):
 self.breaches = breaches.sortedByName()
 }
 
 completion(result)
 }
 }
}

In this example, the view controller is responsible for the view, the presenter/view model is a small testable class, and network and API logic are encapsulated in separate services.

See https://github.com/robertmryan/Breaches for working demonstration.