Refactor for dependency injection

[konnork]

Specifically refactor use of APIManager to utilize dependency injection and get rid of Notification Center in favor of less loosely-coupled communication such as delegates

[manavgabhawala]

I disagree. First of all, the APIManager has no dependencies, it's a singleton because it serves as a single source of truth for the model values. The way I see it there are 4 options to change the information distribution:

1. Inject object/view which is interested in updates. This is a non-starter because of too much code coupling.

2. Delegates. This is okay in some scenarios but I considered this very carefully and opted not to use this because it doesn't support a 1 to many pattern. Finally, it still has the same problems with code coupling where protocols need to be implemented by view controllers and only one view or view controller can be notified of updates. We could use an array of delegates, but that gets too unwieldy too quickly and again serves no benefit over NC.

3. KVO: This supports everything NC does, but serves no benefits over what we currently have. The downside of KVO is having to make everything dynamic and NSObjects everywhere. Too much reliance on the Objc runtime.

4. Notification Center: This is what we are using, of course. Publish-subscribe has lots of benefits for code-decoupling. It is pretty easy to use and its overhead isn't too bad. Some consider this too "loosely coupled", however, it's not in this particular case. There are events that multiple parts of the app should be made aware of, for example logging out/logging in. And this serves what we want to achieve just fine. Additionally, the biggest problem with NC is the loss of type safety, but we never fall into that trap because we only use it to notify subscribers, we never embed data into the notification object. If the subscribers want information they simply query the APIManager again.

So, the way I see it delegates offers no benefits over Notification Center. People advise against misuse of NC, but usually thats when you abuse it and use it everywhere. We have a very limited list of notifications and there's a very good reason we are using it.

[konnork]

I wasn't saying APIManager needs dependency injection, I was saying I would like to dependency inject a reference to the APIManager down the view hierarchy, rather than use a singleton :P

New question: What are your thoughts on splitting up some of the related functions (like the Floor related functions) into their own objects to inject into view controllers?

[manavgabhawala]

We can do that, my only question is why? Is this somehow better than a singleton, I think not. Additionally we would have to store a bunch of references to the same APIManager in any case which would mean unnecessary ref counting and just more code for no gain.

Are there that many functions other than retrieving an image from a floor? I can't think of any but I may be wrong. If that is the only function, we could still do this but again, why? It doesn't serve any purpose. Finally, it is definitely cleaner to have a "Floor" retrieve its on image.

[konnork]

Passing a reference to the same object is extra work with little gain. My goal is to only pass down relevant pieces to each view controller hierarchy (e.g. Calendar doesn't need to even have access to functions related to the countdown).

The purpose is to split up the 750 line APIManager God class into smaller pieces. At the very least into a different files if not different objects

[russellladd]

The real reason I suggested we use delegates over NotificationCenter was so the APIManager could keep track of who was observing it. We could allow view controllers to start observing on init and stop on deinit, and offer a "pause" feature on viewAppear/Disappear. The pause feature would only send a change notification on an unpause if changes had in fact happened since the pause started.

This API would remove code that is duplicated across all our view controllers and fix some performance bugs we had to hack around (looking at the calendar).

Breaking up the APIManager into multiple classes I'm iffy on.

Making it not a singleton I'm theoretically in favor of, but only if it can be passed through the ViewController initializers, which is only possible if we use custom initializers, which means we can't use Storyboards.

[manavgabhawala]

The real reason I suggested we use delegates over NotificationCenter was so the APIManager could keep track of who was observing it. We could allow view controllers to start observing on init and stop on deinit, and offer a "pause" feature on viewAppear/Disappear. The pause feature would only send a change notification on an unpause if changes had in fact happened since the pause started.

There's no reason to use delegates for that. That's essentially asking for a pause feature in NC. Which would entail making our own NC which although feasible, I lean slightly on against doing.

Passing a reference to the same object is extra work with little gain. My goal is to only pass down relevant pieces to each view controller hierarchy (e.g. Calendar doesn't need to even have access to functions related to the countdown).

Splitting things up in this manner will make future changes very difficult. Lets say we want to change calendar to show the countdown sometime later, or somehow integrate announcements with the calendar, no access to the model will make it very cumbersome to change. This also serves no real functional purpose, imo.

The purpose is to split up the 750 line APIManager God class into smaller pieces. At the very least into a different files if not different objects

Breaking up the APIManager into multiple classes I'm iffy on.

Same, I disagree with the premise that the APIManager is a god class. Rather than LOC, you should look at public interface (also 750 is nothing). How long or detailed is that. Currently, its not too bad. If the APIManager requires a lot of private code to implement its behavior, so be it. Also how are you going to refactor this code to achieve the same amount of code reuse as the APIManager currently has?