Skip to content
This repository has been archived by the owner on May 6, 2024. It is now read-only.

Create dedicated coordinator to manage view controller transitions #29

Open
konnork opened this issue Apr 11, 2017 · 4 comments
Open

Create dedicated coordinator to manage view controller transitions #29

konnork opened this issue Apr 11, 2017 · 4 comments

Comments

@konnork
Copy link
Member

konnork commented Apr 11, 2017

Depends on #26

Currently we are using storyboards with segues to manage view controller transitions

I propose moving to a dedicated coordinator object to handle transitions between view controllers. This AppCoordinator will be instantiated in the AppDelegate and manage all view controllers. It's initializer will create the initial view controller hierarchy (e.g. UITabBarController -> UINavigationController -> UIViewController (x5)).

It will have a hierarchical coordination system:

AppCoordinator:
CalendarCoordinator
MapsCoordinator
CountdownCoordinator
AnnouncementsCoordinator
TicketCoordinator

Each view controller with transition activating events (like tapping a table view cell) will have its own coordination delegate defined with methods such as calendarViewController:didSelectEvent: that will be implemented by its designated coordinator to manage the instantiation and transition to EventViewController with that particular event.

Goals

  • Prevent view controllers from knowing their surrounding context
  • Isolate view controllers for clean unit/UI testing
  • Allow for easy restructuring of app view hierarchy for flow changes and easier testing during development
@konnork
Copy link
Member Author

konnork commented Apr 11, 2017

@russellladd @manavgabhawala Thoughts?

@russellladd
Copy link
Contributor

I don't understand why this is necessary.

First point: View controllers should already be oblivious to their surrounding context (i.e. they don't know how they got on screen). View controllers are responsible for getting other view controllers on screen (downstream view controllers), but I don't see a good reason to separate this concern into it's own class.

Second and third points: I think getting rid of storyboards should already go a long way to making these achievable, no?

I do think there is an interesting idea here, especially since I assume the CalendarCoordinator would wrap the UINavigationController + CalendarViewController + EventViewController all into one package.

I feel like this would only serve to add 6 more classes and shuffle code around. Moreover, I don't think it would make the code easier to understand for us, and it would definitely make it harder to onboard new people.

@manavgabhawala
Copy link
Member

I don't see the purpose of making a distinction between VCs and coordinators. How are they different?

Also just from the goals:

  • Prevent view controllers from knowing their surrounding context
    • Already done, VCs don't have or know about surrounding contexts by design.
  • Isolate view controllers for clean unit/UI testing
    • View controllers are already isolated enough for unit and UI testing. Also for reusability, for example I reused lots of the view controller stuff for the widget. Plus I know we should test on paper, but in practice we currently have 0 tests. First lets see some tests and only then if we feel the need, should we consider a restructure like this.
  • Allow for easy restructuring of app view hierarchy for flow changes and easier testing during development
    • I don't think having VCs and coordinators split like this serve any purpose than add number files and more code is always more difficult to maintain.

@konnork
Copy link
Member Author

konnork commented Apr 14, 2017

My bad I wasn't clear, when I said surrounding context, I exclusively was referring to downstream view controllers. Personally, I like to prevent view controllers from doing anything besides setting up their view and responding to events (and delegating anything they don't have complete ownership of). I admit I'm a bit on the extreme side on that.

Your point on tests is completely valid, I'm going to make a (inevitably unsuccessful) push for testing this summer, so we will see what happens there

Saying they don't "serve any purpose" might be a bit harsh. I admit it isn't currently hindering development at all, I just find it cleaner.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants