Skip to main content
Code Review

Return to Answer

simply code snippet
Source Link
Rob
  • 2.7k
  • 16
  • 27
class ViewController: UITableViewController {
 private let viewModel = BreachViewModel()
 private var dataSource: UITableViewDiffableDataSource<Section, Breach>!

 override func viewDidLoad() {
 super.viewDidLoad()
 configureTableView()
 }
 override func updateProperties() {
 updateTableView()
 }
}

A more complete example can be found on GitHub . I’m just going against https://httpbin.org/json (so I’ve contorted myself to take their "slide show" data model and relabel it as "breaches"), but it illustrates the basic division of labor that I employ above.

class ViewController: UITableViewController {
 private let viewModel = BreachViewModel()
 private var dataSource: UITableViewDiffableDataSource<Section, Breach>!

 override func viewDidLoad() {
 super.viewDidLoad()
 configureTableView()
 }
 override func updateProperties() {
 updateTableView()
 }
}
class ViewController: UITableViewController {
 private let viewModel = BreachViewModel()
 override func viewDidLoad() {
 super.viewDidLoad()
 configureTableView()
 }
 override func updateProperties() {
 updateTableView()
 }
}

A more complete example can be found on GitHub . I’m just going against https://httpbin.org/json (so I’ve contorted myself to take their "slide show" data model and relabel it as "breaches"), but it illustrates the basic division of labor that I employ above.

Retire `statusLabel` from earlier example
Source Link
Rob
  • 2.7k
  • 16
  • 27
class ViewController: UITableViewController {
 @IBOutlet private weak var statusLabel: UILabel!
 private let viewModel = BreachViewModel()
 private var dataSource: UITableViewDiffableDataSource<Section, Breach>!
 override func viewDidLoad() {
 super.viewDidLoad()
 configureTableView()
 }
 override func updateProperties() {
 updateTableView()
 }
}
protocol NetworkManagerProtocol {
 func data(from url: URL) async throws -> Data
}
class NetworkManager: NetworkManagerProtocol {
 static let shared = NetworkManager()
 enum HTTPError: Error {
 case invalidURL
 case invalidResponse(Data, URLResponse)
 }
 public func data(from url: URL) async throws -> Data {
 let (data, response) = try await URLSession.shared.data(from: url)
 guard
 let response = response as? HTTPURLResponse,
 200 ..< 300 ~= response.statusCode
 else {
 throw HTTPError.invalidResponse(data, response)
 }
 return data
 }
}
class ViewController: UITableViewController {
 @IBOutlet private weak var statusLabel: UILabel!
 private let viewModel = BreachViewModel()
 private var dataSource: UITableViewDiffableDataSource<Section, Breach>!
 override func viewDidLoad() {
 super.viewDidLoad()
 configureTableView()
 }
 override func updateProperties() {
 updateTableView()
 }
}
protocol NetworkManagerProtocol {
 func data(from url: URL) async throws -> Data
}
class NetworkManager: NetworkManagerProtocol {
 static let shared = NetworkManager()
 enum HTTPError: Error {
 case invalidURL
 case invalidResponse(Data, URLResponse)
 }
 public func data(from url: URL) async throws -> Data {
 let (data, response) = try await URLSession.shared.data(from: url)
 guard
 let response = response as? HTTPURLResponse,
 200 ..< 300 ~= response.statusCode
 else {
 throw HTTPError.invalidResponse(data, response)
 }
 return data
 }
}
class ViewController: UITableViewController {
 private let viewModel = BreachViewModel()
 private var dataSource: UITableViewDiffableDataSource<Section, Breach>!
 override func viewDidLoad() {
 super.viewDidLoad()
 configureTableView()
 }
 override func updateProperties() {
 updateTableView()
 }
}
protocol NetworkManagerProtocol {
 func data(from url: URL) async throws -> Data
}
class NetworkManager: NetworkManagerProtocol {
 static let shared = NetworkManager()
 enum HTTPError: Error {
 case invalidResponse(Data, URLResponse)
 }
 public func data(from url: URL) async throws -> Data {
 let (data, response) = try await URLSession.shared.data(from: url)
 guard
 let response = response as? HTTPURLResponse,
 200 ..< 300 ~= response.statusCode
 else {
 throw HTTPError.invalidResponse(data, response)
 }
 return data
 }
}
Update for modern Observation patterns; retire the outdated manual "binding" of view model objects to the view controller with closures
Source Link
Rob
  • 2.7k
  • 16
  • 27
  • 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.

    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 unit 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 presenter or whatever you want to call it), should not be UIKit-specific, and it’s that mediator on which we’ll do our unit testing.

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".

  • While this distinction is often ignored (with the term "view model" often being used quite loosely), technically, MVVM generally suggests that you’re doing someone would employ "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, technically, they’re just different.

    Quite frankly, many use the term of "view model" within the UIKit world quite loosely, often applying the term that are actually following something that is technically closer to MVP.

  • 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 strive to be UIKit dependent, and it definitely shouldn’t. It really should not 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.

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.

    • 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 ). If you adopt async-await, it prevents this class of mistake.

    • 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.

Generally, if you were goingreally determined to writeadopt MVVM (as opposed to MVP-style patterns), you’d use a framework like SwiftUI Bond (with ObservableObject or Observation framework) or SwiftRX (or possibly the older Bond ) to facilitate the bindings. But let’s contemplate what a minimalist implementation might look like.

In iOS 18, you can use (In the absence of these binding networksObservation framework, this is arguably more MVP than MVVMtoo, 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 viewit’s inelegant.)

Bottom line In iOS 26, the view controller would basically set up the view model (possibly passing in initial model data)situation has improved, and telloffering the view model what it wanted to donew updateProperties method that will be called automatically when the view model wanted to inform us@Observable object updates:

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)
 }
 }
 }
}
class ViewController: UITableViewController {
 @IBOutlet private weak var statusLabel: UILabel!
 private let viewModel = BreachViewModel()
 private var dataSource: UITableViewDiffableDataSource<Section, Breach>!
 override func viewDidLoad() {
 super.viewDidLoad()
 configureTableView()
 }
 override func updateProperties() {
 updateTableView()
 }
}

And:

import Foundation
import Observation
@Observable
class BreachViewModel {
 var breaches: [Breach] = []
 @ObservationIgnored private let apiManager: ApiManager
 /// Create breaches view model.
 ///
 /// - Parameter apiManager: Optional `ApiManager`. Generally omitted from the call point. But, in unit tests, you might instantiate the view model with:
 ///
 /// let viewModel = BreachViewModel(
 /// apiManager: ApiManager(networkManager: mockedNetworkManager)
 /// )
 init(apiManager: ApiManager = .shared) {
 self.apiManager = apiManager
 }
 func fetchBreaches() async throws {
 self.breaches = try await apiManager.breaches()
 }
}

And

class ApiManager {
 static let shared = ApiManager()
 private let baseUrl = URL(string: ...)!
 private let breachesPath = "..."
 private let networkManager: NetworkManagerProtocol
 init(networkManager: NetworkManagerProtocol = NetworkManager.shared) {
 self.networkManager = networkManager
 }
 func breaches() async throws -> [Breach] {
 let url = baseUrl.appending(path: breachesPath)
 let data = try await networkManager.data(from: url)
 let responseObject = try JSONDecoder().decode(BreachResponse.self, from: data)
 return responseObject.payload.breaches // the specifics of how you extract `breaches` from your payload will obviously vary
 }
}

And

protocol NetworkManagerProtocol {
 func data(from url: URL) async throws -> Data
}
class NetworkManager: NetworkManagerProtocol {
 static let shared = NetworkManager()
 enum HTTPError: Error {
 case invalidURL
 case invalidResponse(Data, URLResponse)
 }
 public func data(from url: URL) async throws -> Data {
 let (data, response) = try await URLSession.shared.data(from: url)
 guard
 let response = response as? HTTPURLResponse,
 200 ..< 300 ~= response.statusCode
 else {
 throw HTTPError.invalidResponse(data, response)
 }
 return data
 }
}

The view model could havedetails matter less than a method to perform the request and inform the view (controller)clear separation of the changesresponsibilities:

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)
 }
 }
}
  • The view controller is responsible for managing the UI;
  • The view model is a thin, OS-independent, layer of model objects necessary to support the view controller and initiates interactions with other services;
  • The API layer is where you embed things like endpoints, authentication, request preparation, and the parsing responsibilities; and
  • The network layer (which should be protocol-based to support mocking for tests) is where you perform the actual network requests.

In this example, the view controllerThis UIKit rendition still isn’t technically MVVM (because you still have to manually observe model updates and trigger UI updates; there is responsible for the viewno automatic two-way binding of controls to model objects), but the presenter/view model isObservation framework has evolved to a small testable class, and network and API logic are encapsulated in separate servicespoint that you can minimize much of the noise previously associated with UIKit projects.

See https://github.com/robertmryan/Breaches for working demonstration If you want true MVVM data-binding, SwiftUI offers more natural integration.

  • 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.

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.

  • 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 unit 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 presenter or whatever you want to call it), 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".

  • While this distinction is often ignored (with the term "view model" often being used quite loosely), technically, MVVM suggests that one would employ "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, technically, they’re just different.

    Quite frankly, many use the term of "view model" within the UIKit world quite loosely, often applying the term that are actually following something that is technically closer to MVP.

  • 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 strive to be UIKit dependent. It really should not 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 ). If you adopt async-await, it prevents this class of mistake.

    • 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.

Generally, if you were really determined to adopt MVVM (as opposed to MVP-style patterns), you’d use a framework like SwiftUI (with ObservableObject or Observation framework) or SwiftRX (or possibly the older Bond ) to facilitate the bindings.

In iOS 18, you can use Observation framework, too, but it’s inelegant. In iOS 26, the situation has improved, offering the new updateProperties method that will be called automatically when the @Observable object updates:

class ViewController: UITableViewController {
 @IBOutlet private weak var statusLabel: UILabel!
 private let viewModel = BreachViewModel()
 private var dataSource: UITableViewDiffableDataSource<Section, Breach>!
 override func viewDidLoad() {
 super.viewDidLoad()
 configureTableView()
 }
 override func updateProperties() {
 updateTableView()
 }
}

And:

import Foundation
import Observation
@Observable
class BreachViewModel {
 var breaches: [Breach] = []
 @ObservationIgnored private let apiManager: ApiManager
 /// Create breaches view model.
 ///
 /// - Parameter apiManager: Optional `ApiManager`. Generally omitted from the call point. But, in unit tests, you might instantiate the view model with:
 ///
 /// let viewModel = BreachViewModel(
 /// apiManager: ApiManager(networkManager: mockedNetworkManager)
 /// )
 init(apiManager: ApiManager = .shared) {
 self.apiManager = apiManager
 }
 func fetchBreaches() async throws {
 self.breaches = try await apiManager.breaches()
 }
}

And

class ApiManager {
 static let shared = ApiManager()
 private let baseUrl = URL(string: ...)!
 private let breachesPath = "..."
 private let networkManager: NetworkManagerProtocol
 init(networkManager: NetworkManagerProtocol = NetworkManager.shared) {
 self.networkManager = networkManager
 }
 func breaches() async throws -> [Breach] {
 let url = baseUrl.appending(path: breachesPath)
 let data = try await networkManager.data(from: url)
 let responseObject = try JSONDecoder().decode(BreachResponse.self, from: data)
 return responseObject.payload.breaches // the specifics of how you extract `breaches` from your payload will obviously vary
 }
}

And

protocol NetworkManagerProtocol {
 func data(from url: URL) async throws -> Data
}
class NetworkManager: NetworkManagerProtocol {
 static let shared = NetworkManager()
 enum HTTPError: Error {
 case invalidURL
 case invalidResponse(Data, URLResponse)
 }
 public func data(from url: URL) async throws -> Data {
 let (data, response) = try await URLSession.shared.data(from: url)
 guard
 let response = response as? HTTPURLResponse,
 200 ..< 300 ~= response.statusCode
 else {
 throw HTTPError.invalidResponse(data, response)
 }
 return data
 }
}

The details matter less than a clear separation of responsibilities:

  • The view controller is responsible for managing the UI;
  • The view model is a thin, OS-independent, layer of model objects necessary to support the view controller and initiates interactions with other services;
  • The API layer is where you embed things like endpoints, authentication, request preparation, and the parsing responsibilities; and
  • The network layer (which should be protocol-based to support mocking for tests) is where you perform the actual network requests.

This UIKit rendition still isn’t technically MVVM (because you still have to manually observe model updates and trigger UI updates; there is no automatic two-way binding of controls to model objects), but the Observation framework has evolved to a point that you can minimize much of the noise previously associated with UIKit projects. If you want true MVVM data-binding, SwiftUI offers more natural integration.

add code snippets
Source Link
Rob
  • 2.7k
  • 16
  • 27
Loading
Source Link
Rob
  • 2.7k
  • 16
  • 27
Loading
default

AltStyle によって変換されたページ (->オリジナル) /