willowtreeapps / cordux

https://willowtreeapps.github.io/cordux/
MIT License
61 stars 4 forks source link

Remove advice that the coordinator should act as a view controller's handler #26

Open ianterrell opened 7 years ago

ianterrell commented 7 years ago

After devoting about 35 kloc to this paradigm, I think it's worth revisiting whether or not the coordinator should be a view controller's handler.

Mr. Khanlou's inspiring post on coordinators had the coordinator act as a delegate for view controllers. I adopted the term "handler" since "delegate" felt heavyweight, and I just wanted to handle user input. I used short action names like signIn() instead of signInFromViewController(viewController: AuthenticationViewController). However, in development, it seemed like the short terms were confusing and clashing when a coordinator had multiple responsibilities or managed multiple view controllers, each with their own context-less handler methods.

Additionally, the delegate pattern creates a reference cycle from the coordinator to the view controller (strong) and the view controller to its delegate coordinator (weak). That's fine (ish) when it's only used in that capacity, but we've coded at least one case where the delegate got passed around to be used by owned types and their views (nested collection views where each one manages itself). That made the object graph very confusing and resulted in a strong cycle.

I think it is better engineering to recommend separate types implement the handler protocol. This leads to several improvements.

  1. We don't need to have a reference cycle of any sort. The view controller can reference the handler and the coordinator can reference the handler (if necessary), but the handler will not reference either of those types.
  2. Without the reference cycle, there is no need for any relationship to be weak.
  3. Without the need for a weak reference, we can remove the class restriction on handler protocols.
  4. If a new type is built for each handler, there is no overload of a type being multiple handlers and therefore there is no problem with using terse names like signIn(), as their meaning will be clear from context.
  5. It better adheres to the SRP.
  6. It reduces the likelihood of changing the problem of Massive View Controller to Massive Coordinator.
ianterrell commented 7 years ago

Recommended object graph:

coordinator
   | 
   v
view controller
   | 
   v 
handler  

or

coordinator --> handler
   |            ^
   v           /
view controller

if the coordinator needs access to the handler at any point.