MVVM-C Swift with Dependency injection

Question

I want to create a MVVM-C project, but also adequately test it. Naturally I want to create such a project that can access a Network Service.

Any comments or thoughts on this approach?

DependencyFactory:

protocol Factory {
    var networkManager: HTTPManagerProtocol { get }
    func makeInitialViewModel(coordinator: Coordinator) -> InitialViewModel
    func makeInitialView(viewModel: InitialViewModel) -> InitialView
    func makeDetailView(viewModel: DetailViewModel) -> DetailView
    func makeDetailViewModel(coordinator: Coordinator) -> DetailViewModel
}

// replace the DependencyContainer for tests
class DependencyFactory: Factory {
    var networkManager: HTTPManagerProtocol = HTTPManager()
    
    // should not return an optional at the end of this project
    func makeInitialCoordinator() -> ProjectCoordinator {
        let coordinator = ProjectCoordinator(factory: self)
        return coordinator
    }
    
    func makeInitialView(viewModel: InitialViewModel) -> InitialView {
        let view = InitialView()
        return view
    }
    
    func makeInitialViewModel(coordinator: Coordinator) -> InitialViewModel {
        let viewModel = InitialViewModel(coordinator: coordinator, networkManager: networkManager)
        return viewModel
    }
    
    func makeDetailView(viewModel: DetailViewModel) -> DetailView {
        let view = DetailView()
        return view
    }
    
    func makeDetailViewModel(coordinator: Coordinator) -> DetailViewModel {
        let viewModel = DetailViewModel(coordinator: coordinator, networkManager: networkManager)
        return viewModel
    }
}

A basic HTTPManager (that doesn't really touch the network, but you get the point!)

protocol HTTPManagerProtocol {
    func get(url: URL, completionBlock: @escaping (Result<Data, Error>) -> Void)
}

class HTTPManager: HTTPManagerProtocol {
    public func get(url: URL, completionBlock: @escaping (Result<Data, Error>) -> Void) {
        DispatchQueue.main.asyncAfter(deadline: .now() + 2) {
            let data = Data("The Data from HTTPManager".utf8)
            completionBlock(.success(data))
        }
    }
}

Project Coordinator

protocol Coordinator: class {
    func start(_ navigationController: UINavigationController)
    func moveToDetail()
}

class ProjectCoordinator: Coordinator {
    var childCoordinators = [Coordinator]()
    var navigationController: UINavigationController?
    
    private var factory: Factory

    init(factory: Factory) {
        self.factory = factory
    }

    func start(_ navigationController: UINavigationController) {
        let vc = InitialViewController(factory: factory, coordinator: self)
        self.navigationController = navigationController
        navigationController.pushViewController(vc, animated: true)
    }
    
    func moveToDetail() {
        let vc = DetailViewController(factory: factory, coordinator: self)
        navigationController?.pushViewController(vc, animated: true)
    }
}

InitialModel

struct InitialModel : Codable {
    let dataString : String
}

InitialView

final class InitialView: UIView {

    let traverseButton = UIButton(type: .custom)
    let networkButton = UIButton(type: .custom)
    let networkLabel = UILabel()
    override init(frame: CGRect) {
        super.init(frame: frame)
        setup()
    }
    
    required init?(coder: NSCoder) {
        fatalError("init(coder:) has not been implemented")
    }
    
    private func setup() {
        self.backgroundColor = .red
        traverseButton.frame = CGRect(x: 0, y: 0, width: 200, height: 100)
        traverseButton.setTitle("Go to Detail", for: .normal)
        traverseButton.setTitleColor(.black, for: .normal)
        traverseButton.isUserInteractionEnabled = true
                
        self.addSubview(traverseButton)
        traverseButton.translatesAutoresizingMaskIntoConstraints = false
        traverseButton.centerXAnchor.constraint(equalTo: self.centerXAnchor).isActive = true
        traverseButton.centerYAnchor.constraint(equalTo: self.centerYAnchor).isActive = true
        
        networkButton.frame = CGRect(x: 0, y: 0, width: 200, height: 100)
        networkButton.setTitle("Make Network Call", for: .normal)
        networkButton.setTitleColor(.black, for: .normal)
        networkButton.isUserInteractionEnabled = true
        
        self.addSubview(networkButton)
        networkButton.translatesAutoresizingMaskIntoConstraints = false
        networkButton.centerXAnchor.constraint(equalTo: self.centerXAnchor).isActive = true
        networkButton.centerYAnchor.constraint(equalTo: self.centerYAnchor, constant: 100).isActive = true
        
        networkLabel.text = "No network calls made"
        networkLabel.backgroundColor = .purple
        self.addSubview(networkLabel)
        
        networkLabel.translatesAutoresizingMaskIntoConstraints = false
        networkLabel.centerXAnchor.constraint(equalTo: self.centerXAnchor).isActive = true
        networkLabel.centerYAnchor.constraint(equalTo: self.centerYAnchor, constant: 200).isActive = true
        networkLabel.widthAnchor.constraint(equalToConstant: 300).isActive = true
        networkLabel.heightAnchor.constraint(equalToConstant: 100).isActive = true
    }
    
    func setNetworkLabel(text: String){
        networkLabel.text = text
    }

}

InitialViewController

class InitialViewController: UIViewController {
    private var coordinator: Coordinator?
    private var factory: Factory?
    
    var intialView: InitialView?
    
    lazy var viewModel: InitialViewModel? = {
        return factory?.makeInitialViewModel(coordinator: coordinator!)
    }()
    
    init(factory: Factory, coordinator: Coordinator) {
        self.factory = factory
        self.coordinator = coordinator
        super.init(nibName: nil, bundle: nil)
    }
    
    required init?(coder: NSCoder) {
        fatalError("init(coder:) has not been implemented")
    }
    
    override func loadView() {
        if let initialView = factory?.makeInitialView(viewModel: viewModel!) {
            initialView.traverseButton.addTarget(self, action: #selector(traverseButton(_:)), for: .touchDown)
            initialView.networkButton.addTarget(self, action: #selector(networkButton), for: .touchDown)
            self.intialView = initialView
            self.view = initialView
        }
    }
    
    override func viewDidLoad() {
        super.viewDidLoad()
    }
    
    @IBAction func traverseButton(_ sender: UIButton) {
        coordinator?.moveToDetail()
    }
    
    @IBAction func networkButton(_ sender: UIButton) {
        viewModel?.fetchData(completion: { data in
            switch data {
            case .failure: fatalError()
            case .success(let data):
                if let data = data.first {
                    self.intialView?.setNetworkLabel(text: data.dataString)
                }
            }
        })
    }

}

InitialViewModel

class InitialViewModel {
    private var networkManager: HTTPManagerProtocol?
    init(coordinator: Coordinator?, networkManager: HTTPManagerProtocol) {
        self.networkManager = networkManager
    }
    
    func fetchData(completion: @escaping (Result<[InitialModel], Error>) -> Void) {
        networkManager?.get(url: URL(string: "NOURL")!, completionBlock: { result in
            DispatchQueue.main.async {
                switch result {
                case .failure:
                    completion(.failure(NSError()))
                case .success(let data):
                    if let str = String(data: data, encoding: .utf8) {
                        let model = InitialModel(dataString: str)
                        completion(.success([model]))
                    }
                }
            }
        })
    }
}

DetailViewController

class DetailViewController: UIViewController {
    weak var coordinator: Coordinator?
    private var factory: Factory?

    var detailView: DetailView?
    
    lazy var viewModel: DetailViewModel? = {
        return factory?.makeDetailViewModel(coordinator: coordinator!)
    }()
    
    init(factory: Factory, coordinator: Coordinator) {
        self.factory = factory
        self.coordinator = coordinator
        super.init(nibName: nil, bundle: nil)
    }
    
    required init?(coder: NSCoder) {
        fatalError("init(coder:) has not been implemented")
    }
    
    override func loadView() {
        if let detailView = factory?.makeDetailView(viewModel: viewModel!) {
            self.detailView = detailView
            self.view = detailView
        }
    }

    override func viewDidLoad() {
        super.viewDidLoad()
    }
}

DetailViewModel

class DetailViewModel {
    private var networkManager: HTTPManagerProtocol?
    init(coordinator: Coordinator?, networkManager: HTTPManagerProtocol) {
        self.networkManager = networkManager
    }
}

DetailView

final class DetailView: UIView {
    override init(frame: CGRect) {
        super.init(frame: frame)
        setup()
    }
    
    required init?(coder: NSCoder) {
        fatalError("init(coder:) has not been implemented")
    }
    
    private func setup() {
        self.backgroundColor = .blue
    }
}

Answer 1

Factories/DI

Split into modules or something. For now OK, but if your app gets bigger and your single factory depends on everything it becomes rather hard to maintain. Could be as easy as adding extensions to your Factory protocol for every module. You still have everything in the same class, but your source code is more separated.

makeInitialView, makeDetailView: Get rid of the viewModel parameter as it is not used. You might even consider directly creating your views in the view controller as they have no other dependencies.

Coordinators

You should split your Coordinator protocol into a abstract coordinator protocol and a protocol for the project coordinator. Like this each child coordinator you add to your ProjectCoordinator also needs to have a moveToDetail() method. This is probably not what you want.

Passing the factory to the root coordinator is a good practice. Passing it on to the view controllers from there is not a good idea though. Better to have the factory create the View Controller itself. So you may want to add an makeInitialViewController(coordinator: Coordinator) -> InitialViewController method.

View Controllers

As I said, they should not know about the factory. The properties for their dependencies (coordinator and factory) should not be optional. You cannot construct the ViewController without them (the init-parameters are not optional) so there is no need to deal with with the fact that they are optional.

The exception might be the Coordinator, but it also should be weak. Currently you have a strong retain cycle here (ProjectCoordinator (1) -> NavigationController -> InitialViewController -> ProjectCoordinator (1)). This does not matter, there will be no memory leak as the cycle will be broken at when the view controller is popped from the navigation stack. You can arrange this for every coordinator and get away with the cycles, but long-term it will be easier to just always use weak references from the view controller back to its coordinator. Its easy to forget breaking that cycle.

You deal with the optionals by force-unwrapping (which in your case should be fine), but this makes the code really hard to understand. Every time you see the ! you must mentally check whether this is OK or a crashing bug waiting to happen. If you are not careful and accept them as okay it is only a matter of time until you create a crash. If there is no choice but to force-unwrap something you should always add a comment that explains why it is not a problem.

To support the lazy creation of the view model you might want to inject a Provider for your view model instead of the factory. This could be as simple as a closure: typealias Provider<T> = () -> T

Networking

You should add [weak self] in your callback closure. If the user navigates away from your screen before the request finishes you shouldn’t do any extra work to set up views that are not visible anyways. It would be even better to cancel the request when the user navigates away. No need to waste bandwidth for data the user is not going to see. But this depends on your exact flow and use case.

Small things:

Not really important, but here are some small things that should be fixed:

You shouldnt create a NSError() like this. Even shows this message in the console:

-[NSError init] called; this results in an invalid NSError instance. It will raise an exception in a future release. Please call errorWithDomain:code:userInfo: or initWithDomain:code:userInfo:. This message shown only once.

Better just return nil in required init?(coder: NSCoder) than calling fatalError. You also can mark them as unavailable @available(*, unavailable) so you dont ahve to deal with that in subclasses.

Activate all constraints at the same time using NSLayoutConstraint.activate([]) instead of activating each seperately.

Remove your override-Methods if they do nothing but call super. Its just unneccesary clutter.