Need technical advice about passing data through UINavigationController

Question

First of all a small introduction, im relatively new to Swift and to programming in general, been doing it for the last year and loving every and each new thing of this vast world.

My post is about some technical advices and to know if the decisions that are being made in my company makes any sense.
I will first address the design that is being suggested and next my conclusions.

The design that's being implemented;

We are working in a big app, this app has some flows where they follow a sequence of 5 to 8 controllers, our team leader insists in this design pattern; 
Let’s call the first controller a holder and the holder(black border) is responsible to have a container, this container has a proper navigation controller(red border), also, the holder hold all the data(orange) that those secondary controllers are generating.

Diagram of what this pattern is trying to achieve

To do this every red border controller has a method:



private func getParent() -> HolderViewController? {

    if let parent = navigationController?.parent as? HolderViewController {

        return parent
    }
    return nil
}

And to write in holder we call the method;

getParent().someModel.someModelProperty = "some data”

Conclusion;

Passing data through navigation controller seems to go against to the single responsibility principle. Creating strong references in every controller, even if I ensure that the navigationController is properly deinit when flow ends, does not seem a good option, this could lead to memory leaks and retain cycles. I cannot ensure that, for some hod reason, two controllers could try to access the same property at the same time. This seems the Singleton Design pattern but with a limited “scope”

Resolutions;

Since I am new and I’m working in a company, and, every company has a hierarchy my objective above all is to learn if my conclusions are wrong and have better and more concise explanation about this.

First of all, to address the problem of memory leaks I created a concurrentQueue. Instead of accessing the model directly to write in it I tried to address it through a method that will use a keyPath instead of the model directly, this is the method I’m using to write in the model;

In holder:

class HolderViewController: UIViewController {

   @IBOutlet weak var bottomNavigationContainer: UIView!

   private var bottomNavigationController: UINavigationController!
   private var someModel: SomeModel!
   private let concurrentQueue: DispatchQueue = DispatchQueue(label: "concurrentQueue", attributes: .concurrent)

    override func viewDidLoad() {
        super.viewDidLoad()

        setupBottomNavigation()
    }

    private func setupBottomNavigation() {

        rootController = SecondayViewController()

        if let root = rootController {

            bottomNavigationController = UINavigationController(rootViewController: root)
            addChild(bottomNavigationController)
            bottomNavigationController.view.frame = bottomNavigationContainer.bounds
            bottomNavigationContainer.addSubview(bottomNavigationController.view)
        }
    }
}

extension HolderViewController {

    public func setValueInModel<Value>(_ value: Value, forKey path: WritableKeyPath<SomeModel, Value>) {

       concurrentQueue.async(flags: .barrier) { [weak someModelInstance] in

            if var obj = someModelInstance {

                obj[keyPath: path] = value
            }
        }
    }

    public func readFromHolder() -> SomeModel {

        concurrentQueue.sync {
            return self.someModelInstance
        }
    }
}

The red border controllers are the ones inside the bottom navigation controller;

class RedBorderViewController: UIViewController {

    var someString: String?

    @IBOutlet weak var textField: UITextField!

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

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

      if let text = readFromHolder() {

        textField.text = text
      }
    }

    @IBAction func continueButton(_ sender: Any) {

      if let text = textField.text {

        setValueInHolder(text)
      }
    }

    private func getParent() -> HolderViewController? {

      if let parent = navigationController?.parent as? HolderViewController {

        return parent
      }
      return nil
    }

    private func setValueInHolder(string: String) {

      getParent().setValueInModel(string, forKey: \.someModelProperty)

    }

    private func readFromHolder() -> String {

       return getParent().readFromHolder().someModelProperty
    }

} 

the code above is just a example of how we doing things

This look like some messy code to do a simple thing, we could use closures, delegates, segues etc… but my team leader does not like the other, simpler and more common solutions. Forgot to mention, every of our controllers has a xib and we do not use storyboards.
I know the basics of how to use the other options, what I’m trying is to understand if this is or it isn’t a good solution and why, and if my way of thinking and my methods make any sense.

Remember, every conclusion I took or every solution I've implemented could be wrong, that’s why I’m sharing with you my thoughts in order to learn from your advices and experience

Thanks in advance. :) Stay safe!

Answer 1

First things first: you shouldn't really need to worry about memory leaks here via the someModel property, since as far as I can tell someModel isn't holding on to any reference from the containing HolderViewController. Your concurrentQueue stuff would protect against someModel being modified from multiple threads, but from what I can tell all the modification is happening via an IBAction anyway, so all the action will be on the main thread.

As far as architecture goes, I would probably approach this by making the HolderViewController a more integral part of the navigation flow of the RedBorderController (by either subclassing UINavigationController, or implementing HolderViewController as a wrapper around UINavigationController). Just like UINavigationController.pushViewController(_:animated:) sets the navigationController property on the pushed view controller, you could have something like this:

class HolderViewController: UINavigationController {
    // ...
}

class RedBorderViewController: UIViewController {
    // ...
    var holderViewController: HolderViewController? {
        self.navigationController as? HolderViewController
    }

    @IBAction func continueButton(_ sender: Any) {
      if let text = textField.text {
        setValueInHolder(text)
      }
    }

    private func setValueInHolder(string: String) {
      self.holderViewController.someModel.someModelProperty = string
    }

    private func readFromHolder() -> String {
       return self.holderViewController.someModelProperty
    }
}

If HolderViewController is doing a lot of work outside of just being a container for the UINavigationController, then it will probably be more trouble than it's worth to have it be a subclass. Instead, you can implement it as a wrapper around UINavigationController (like you have now), but have navigation events go through the holder view controller instead of the navigationController:

class HolderViewController: UINavigationController {
    func pushBottomNavViewController(_ viewController: UIViewController, animated: Bool) {
        self.bottomNavigationController.pushViewController(viewController, animated: animated)
        if let redBorderVC = viewController as? RedBorderViewController {
            redBorderVC.holderViewController = self
        }
    }
}

class RedBorderViewController: UIViewController {
    // ...
    weak var holderViewController: HolderViewController?

    @IBAction func continueButton(_ sender: Any) {
      if let text = textField.text {
        setValueInHolder(text)
      }
    }

    private func setValueInHolder(string: String) {
      self.holderViewController.someModel.someModelProperty = string
    }

    private func readFromHolder() -> String {
       return self.holderViewController.someModelProperty
    }

    private func nextScreen() {
        self.holderViewController.pushBottomNavViewController(nextViewController, animated: true)
    }
}

(Note that here, we've made holderViewController a weak var in order to prevent a strong reference cycle via holderViewController -> bottomNavigationController -> viewControllers -> redBorderViewController)

With this approach, you just have to remember to handle navigation events within your RedBorderViewController through holderViewController rather than navigationController, or else holderViewController won't be set on the next item in the stack.

If you want even more abstraction, you could define a protocol like:

protocol RedBorderDataStore: AnyObject {
    var someModel: SomeModel { get set }
}

And instead of holderViewController define a property weak var dataStore: RedBorderDataStore? on RedBorderViewController and handle your model property though that. Then HolderViewController could implement RedBorderDataStore, and pass itself either via the methods I've shown, or the RedBorderViewController could be responsible for handing off the dataStore on creation, e.g. by having an init(dataStore: RedBorderDataStore) initializer.