DeclarativeHub / ReactiveKit

A Swift Reactive Programming Kit
MIT License
1.24k stars 114 forks source link

Best practices regarding shared properties between ViewModels #237

Open LucasVanDongen opened 4 years ago

LucasVanDongen commented 4 years ago

First of all my apologies if architectural questions are not supposed to be asked in the issue tracker but I couldn't find a satisfying answer anywhere else.

In my particular case I have the following two ViewModels:

/** I selected a certain day here in a week **/
class DayViewModel: BaseViewModel {
    let selectedDay: Property<Date>

    func switched(to dayNumber: Int) {
        selectedDay.value = selectedDay.value.dayInSameWeek(for: dayNumber)
    }
}
/** I generate an overview on the home screen depending on which day is selected **/
class HomeViewModel: BaseViewModel, LogsContainingViewModel {
    /** snip **/
    let selectedDay: Property<Date>
    /** snip **/
}

I'm wondering what's the best approach. I could either monitor that property like this:

        daysViewModel.selectedDay.observeNext { [unowned self] newDate in
            self.viewModel.selectedDay.value = newDate
        }.dispose(in: disposeBag)

Or, I pass in the property when I construct the DayViewModel:

        let daysViewModel = DaysViewModel(selectedDay: self.homeViewModel.selectedDay)

On one hand I think having less state is always better, on the other hand the properties might have different life-cycles and I can also imagine the same property shared over multiple view models could cause more problems with threading etcetera.

AnthonyMillerSF commented 4 years ago

Questions like this are probably generally better asked on StackOverflow with the tag reactivekit added to them (if you didn't get any responses there, I'd then create an issue here, linking the question), but no worries.

Answer

I would generally prefer something closer to Option 1, because of possible threading or race condition issues. Your instincts about that are probably correct; sharing mutable state across multiple different places can cause lots of headaches later. It's hard to track and even harder to debug. 9 months from now when you are trying to figure out why your selectedDay on your DayViewModel keeps changing on you, you may not remember immediately that you are sharing that property with the HomeViewModel and that's changing it.

With functional reactive code, try to make your data flow explicit and read in an almost human readable way. Passing one property into the initializer of another view model isn't clear, you now have to look inside of that view model and see whether it's sharing that property, observing it internally, or maybe just using it to get an initial value and not observing it at all.

Instead, having a consistent pattern you follow for where you do all your bindings is better. That could be in some service layer, in your view controller's viewDidLoad, or maybe that is the viewModel's initializer (but then you should pass the entire HomeViewModel into the initializer). Whatever it is, try to keep it consistent so you and other developers always know where to look for the bindings when maintaining your code.

But it's important to recognize that your two options actually currently have a functional difference in behavior.

Option 1

In this case, if the selectedDay on the DayViewModel is changed, it won't update the day on the HomeViewModel.

In order to have it update in both directions, you would want to use bidirectionalBind().

Also, for simplicity, you can use

self.viewModel.selectedDay.selectedDay.bind(to: daysViewModel.selectedDay)

// or for bidirectional binding:
self.viewModel.selectedDay.bidirectionalBind(to: daysViewModel.selectedDay)

Option 2

If you want a bidirectional binding, Option 2 does that already. Sharing one property means that if it updates in either place, it will be updated in both. But again, I don't like this shared state here, and with bidirectionalBind(), this can be easily done in a way that explicitly shows that that is what it does and that is what you intended it to do.

Extra: Read-Only Properties

In your DayViewModel, it looks like you are using a function to update the value of selectedDay. Realize that Property is mutable by anything that has access to observe it. If you want to only allow certain object to mutate the value then you should look at using AnyProperty, which is a read-only property. AnyProperty allows you to get the value and  observe the property, without being able to set the value. This could look like this in your example.

class DayViewModel: BaseViewModel {
    private let _selectedDay: Property<Date>
    let selectedDay: AnyProperty<Date> { _selectedDay.readOnlyView }

    func switched(to dayNumber: Int) {
        _selectedDay.value = _selectedDay.value.dayInSameWeek(for: dayNumber)
    }
}

Hope all this helps! Good luck!

LucasVanDongen commented 4 years ago

Thanks, this really helped. Especially the AnyProperty is something that comes in really handy since I didn't like the way a regular Property often breaks proper encapsulation. I think I actually looked for it but Googling for terms like "ReactiveKit read-only property" doesn't turn up any results.

I'm going to stick with passing through the values using bind instead of passing the property, so option 1.