irace / architecture

10 stars 0 forks source link

“MVVM”: Controllers, view models, networking, persistence #1

Open irace opened 8 years ago

irace commented 8 years ago

I’m trying to come up with a design in the spirit of MVVM – view controllers are provided with an object that they can observe data changes on, and update in response to user interaction. As such, view controllers know nothing about persistence or networking but do not need to be provided with all of their data upfront.

I don’t want to simply shift a massive view controller’s responsibility into a massive view model, however.

Design goals

final class DashboardCoordinator {
  private let rootViewController: UIViewController
  private let postCache: PostCache
  private let blogCache: BlogCache

  init(rootViewController: UIViewController, postCache: PostCache, blogCache: BlogCache) {
    self.rootViewController = rootViewController
    self.postCache = postCache
    self.blogCache = blogCache
  }

  func start() {
    let viewController = DashboardViewController(dataStore: DashboardDataStore(postCache: postCache, blogCache: blogCache))

    rootViewController.presentViewController(viewController, animated: true)
  }
}

final class DashboardViewController {
    func viewDidLoad() {
        /*
         - Render view data from data store (if present) into view
         - Subscribe to data store updates
         */
    }
}

/**
I envision there being a one-to-one mapping between “data store” classes and corresponding view controllers (could always have a data store implementation that does not use any cache if desired.)

I think many would call this object itself a “view model” but I am trying to keep a distinction between “view data” and networking/persistence.

Data store should query the cache in such a way that if a change is made to the cache from elsewhere in the application, that all data sources whose queries would be impacted can be reloaded. This could be fine-grained (e.g. `NSManagedObjectContextDidChange` notifications) with detailed information, or much coarser.

This is readonly now, but in theory the view controller could make modifications to the underlying data based on user interaction. How should that look? Should this object expose methods like `deletePost(identifier:)`?
*/
final protocol DashboardDataStore {
  /* 
   - Immediately populated with cached data
   - Not observable itself since this wouldn’t be granular enough?
   */
  var dashboardViewModel: DashboardViewModel

  init(postCache: PostCache, blogCache: BlogCache)

  // Fetch new data from network, update cache, update view model property
  func update(latest: Bool = true)
}

protocol DashboardViewModel {
  var dashboardPostViewModels: Observable<[DashboardPostViewModel]> { get }
}

protocol DashboardPostViewModel {
  var attributedText: Observable<String> { get }
  var noteCount: Observable<Int> { get }
  var blogViewModel: Observable<DashboardBlogViewModel> { get }
}

protocol DashboardBlogViewModel {
  var blogName: Observable<String> { get }
  var avatar: Observable<UIImage> { get }
}

// Cache objects are *not* screen specific
final class PostCache {
  /* 
   Implementation unimportant. Could be Core Data, Realm, YapDB, `NSCoding`. 
   Just some way to persist data and get it back later. Not sure if there should be 
   separate “cache” objects per model, or just one overarching DB type.
   */
}
mbbischoff commented 8 years ago

I’ll update this comment with questions / comments as I think of them:

  1. Why is the coordinator responsible for creating the DashboardDataStore? Why not initialize a coordinator with a data source?
  2. I like the thought behind DashboardDataStore, but I don’t like the name since it does a lot more than store a dashboard’s data. I also think it’s silly to call this a view model. I’d probably call this DashboardUpdater (this is what I’m doing in one project), DashboardFetcher, DashboardRepository, or DashboardModelController.
  3. DashboardViewModel: Not sure how this fits in. Definitely wouldn’t want the DashboardDataStore to own/vend it since the DashboardDataStore should be view agnostic. I’d probably have the VC own the DashboardDataStore as you do and then create ViewDatas from some concrete model representation or protocol-ized version of the same thing.
  4. PostCache just don’t like the naming here. PostsObjectStorage, PostsStorage. Got to think about situations where these are more than just a cache.
irace commented 8 years ago

Why is the coordinator responsible for creating the DashboardDataStore? Why not initialize a coordinator with a data source?

I would say outside the scope of what I’m trying to get out of this discussion. You certainly could inject it but I also don’t think doing so buys you much.

I like the thought behind DashboardDataStore, but I don’t like the name since it does a lot more than store a dashboard’s data. I also think it’s silly to call this a view model. I’d probably call this DashboardUpdater (this is what I’m doing in one project), DashboardFetcher, DashboardRepository, or DashboardModelController.

Sure – all of the names in here should be taken with a grain of salt. Updater/Fetcher are OK but don’t really indicate that it can be modified in response to user interaction? Maybe Updater does. Trying to avoid excessive usage of “controller.”

DashboardViewModel: Not sure how this fits in. Definitely wouldn’t want the DashboardDataStore to own/vend it since the DashboardDataStore should be view agnostic. I’d probably have the VC own the DashboardDataStore as you do and then create ViewDatas from some concrete model representation or protocol-ized version of the same thing.

Hmm. Where would the model -> view data production live? Needs to live in either the data store or the view controller, right?

PostCache just don’t like the naming here. PostsObjectStorage, PostsStorage. Got to think about situations where these are more than just a cache.

Yeah I don‘t care about the name. Storage is fine for discussion sake.

Pearapps commented 8 years ago

Hmm. Where would the model -> view data production live? Needs to live in either the data store or the view controller, right?

If the logic is small enough, the ViewController can do it directly (which would be your Binder as well) OR it could be an unrelated type that the VC can talk to.

irace commented 8 years ago

I didn’t really want the VC to know about the models themselves, at all.

Pearapps commented 8 years ago

I didn’t really want the VC to know about the models themselves, at all.

Makes sense

khanlou commented 8 years ago

lgtm

khanlou commented 8 years ago

Just kidding.

It seems like one big thing you're missing here is protocol-ifying your storage. Being able to inject storage into your view controllers is great, but unless you can get some stubs in there for testing, it's ultimately not that valuable. I guess your data source is protocol-ized and it can just ignore whatever storage objects you inject?

I think another thing worth figuring out is a more-or-less complete list of side effects that your view controllers can have, and how those will affect other view controllers. Write to disk, update something on the network, update something in the keychain, anything else? Building a whole big observable architecture for is very cool, but it might be overkill especially since you only have one screen visible at a time. I've never worked with an that had a really effective KVO or RAC or other binding setup, though, so maybe it's secretly insanely good and I just don't know.

Finally, it might be really good to inject your storage objects directly into the view controller and have it construct the data source from those. The benefit is less stuff exposed to the outside world, and you still get the testability benefit.

ashfurrow commented 8 years ago

I like this, agree with @khanlou about using protocols. Keeping it loosely coupled = awesome.

We're trying to figure this out at Artsy, in case anyone is interested in our goals/ideas: https://github.com/artsy/mobile/issues/65

irace commented 8 years ago

Agree @khanlou @ashfurrow – protocols are great and would work swimmingly here – but that’s not what I’m trying to tease apart here. I’m trying to determine:

Thanks for the link @ashfurrow, I’ll take a look.

I think another thing worth figuring out is a more-or-less complete list of side effects that your view controllers can have, and how those will affect other view controllers. Write to disk, update something on the network, update something in the keychain, anything else?

@khanlou My thinking is that my view controllers can only do two things:

Either of this could result in network activity, writing to disk, keychain access, etc., but the VC doesn’t know that. Of course this raises the question of “what should be in the data store vs. in the coordinator (the VC’s delegate)”...

Finally, it might be really good to inject your storage objects directly into the view controller and have it construct the data source from those. The benefit is less stuff exposed to the outside world, and you still get the testability benefit.

Perhaps, though I still think it's preferable for VCs to be given an object that abstracts persistence/network away such that it doesn't care about where data is coming from. In any event, I also don't think this is what I’m trying to get at specifically. Mostly, how do you separate responsibilities of:

Building a whole big observable architecture for is very cool, but it might be overkill especially since you only have one screen visible at a time. I've never worked with an that had a really effective KVO or RAC or other binding setup, though, so maybe it's secretly insanely good and I just don't know.

Not exactly sure what you mean here? Are you simply stating that this goal might not be too important?

View controllers should be able to reflect changes made from a different part of the application

If so, you could definitely be right. I’d put that at a low priority.

mbbischoff commented 8 years ago

@khanlou My thinking is that my view controllers can only do two things:

  • Invoke methods on the data store (as its referred to in this PR). The store vends view models, so changes to the models will need to be reflected in those vended view models, so the VC should tell the data store when the model changes
  • Invoke delegate methods Either of this could result in network activity, writing to disk, keychain access, etc., but the VC doesn’t know that. Of course this raises the question of “what should be in the data store vs. in the coordinator (the VC’s delegate)”...

I strongly agree with this line of thinking. I definitely think there’s a strong move in the community towards view controllers as primarily switchboards that glue together smaller single-purpose objects. I think the coordinator’s job is flow (I’d probably think of it as a FlowController) where the data store/updater/ repository’s job is data access and retrieval. This seems like a pretty good dividing line to me.

khanlou commented 8 years ago

@mattbischoff

I think the coordinator’s job is flow (I’d probably think of it as a FlowController) where the data store/updater/ repository’s job is data access and retrieval. This seems like a pretty good dividing line to me.

The original intent behind coordinators was to make view controllers a) more inert and b) more reusable. They can access whatever data they want, but by presenting one, you can't ruin anyone's data. You mostly get the same benefit here, by injecting the data store (meaning that the view controller is inactive until its owner injects in the power to mutate stuff). However, I think bubbling those changes up to the coordinator and making the coordinator do the destructive mutations is a little better because the view controller is totally agnostic to how its presented or what it's used for.

As an example, a "+" button could be used for presenting a form modally, or it could be used for making an operation on the database (to make a placeholder item) or it could be used for showing a form in-line, like in a table cell. The view controller doesn't know and doesn't need to know which of these will happen when the button is pressed. The view controller just sends up messages about what user input it received, and awaits messages on how to change its state or form.

Now, if you want to test two of those behaviors for the "+" button against each other, you can actually reuse the view controller without putting any state or conditionals in it.