Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor MVVM to a more functional style which avoids Subjects #16

Closed
wants to merge 3 commits into from

Conversation

acchou
Copy link

@acchou acchou commented Feb 8, 2017

As an experiment, I refactored RxTodo a bit to use a somewhat different style of MVVM, would welcome your thoughts on this and its pros and cons.

The core change is to modify ViewModels into two structs representing inputs and outputs (this can also be done with protocols, but I've found that structs work fine for all cases I've seen so far):

struct ViewModelInputs {
  var button: Observable<Void>
}

struct ViewModelOutputs {
  var label: Observable<String>
}

Then we can define a ViewModel as a function:

typealias ViewModel = (ViewModelInputs) -> ViewModelOutputs

We avoid Subjects by passing in the inputs directly in the view controller:

let inputs = ViewModelInputs(button: button.rx.tap.asObservable())
let viewModel = myViewModel(inputs)
let outputs = viewModel(inputs)

outputs.label
  .bindTo(...)
  .addDisposableTo(disposeBag)

There are various benefits to this approach, including not needing to bind each observable separately, a very clear input/output structure, and no need for Subjects which can be an avenue for abuse. There were some additional details I changed along the way:

  • Removed the need for deallocated events and subscriptions within view models (at least explicitly)
  • Added a couple of mock structs for view model inputs to get back Subjects for testing

Overall I think it retains the original structure of RxTodo while making the architecture a bit more purely functional. I'm submitting a pull request as a way to stimulate discussion, I might write a blog post about this if it turns out to be interesting to people.

…ore explicit inputs and outputs, and uses functions and value types.

Also update testsuite for this refactor.
@devxoul
Copy link
Owner

devxoul commented Feb 8, 2017

@acchou, it looks very interesting. I think the key points are:

  1. Separating the Input and Output from the ViewModel
  2. Passing the Output factory method instead of ViewModel instance

It makes sense because ViewModels currently uses their initializers only, which is not really different from using a single function. In order to use existing naming convention, we can define each types as below:

typealias TaskListViewModelType = (TaskListViewModelInput) -> TaskListViewModelOutput
func TaskListViewModel(provider: ServiceProviderType) -> TaskListViewModelType {
  return { input in
    return TaskListViewModelOutput(...)
  }
}

However I'm not sure. Is 'purely functional ViewModel' really good? For me, who doesn't have any experience in functional programming, it looks like a JavaScript mimicking the class behavior with functions. Why do you think it is worth? Here are my thoughts on what you said the benefits are.

  1. a very clear input/output structure

    => We can still achieve it in current architecure just by creating additional Input/Ouput struct types.

  2. no need for Subjects which can be an avenue for abuse

    => I think this is the only benefit.

  3. Removed the need for deallocated events and subscriptions within view models (at least explicitly)

    => Deallocated events are necessary because some observables in your code will never being disposed. (I left comment on it)

//
// Done
//
let createTaskEvent = doneButtonItemDidTap
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

createTaskEvent will never being disposed. Try add .debug() on it.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes you're right. I think one way to handle this is to recreate the view model every time the view is loaded; then we could remove the calls to asInput(), which is causing this problem by removing the propagation of Completion events from the UI controls.

Or, it would be possible to watch for deallocation events, though that still seems a bit messy to me. I'll play with it some more.

…s. No need to watch deallocating events.

In TaskServiceType, ensure subscribed Observables don't complete the event stream. Happens when view models are deallocated.
Remove some stray debug statements from tests.
Remove asInput function, no longer needed. As part of this, remove local variables for inputs in view model creation, no longer needed.
@acchou
Copy link
Author

acchou commented Feb 8, 2017

@devxoul I fixed the leak (without using the deallocation event), and I also changed the naming of the view models to reflect your suggestion.

Regarding the value of this approach, I think it is a matter of style... while theoretically you could have more methods on a view model class, that would mean it could be in different states, and this statefulness would make it more difficult to reason about what the output Observables are going to do.

So the value here seems to be an encouragement to future maintainers to commit to using FRP to express outputs purely functionally in terms of inputs. Let me get some more opinions about this from the RxSwift channel... just want to see if this is worth my time to blog about.

@sunshinejr
Copy link

What I like is the way of splitting the inputs and outputs into multiple types, but for me preferably it would be better if these types were inside the view model. That being said, the lines where you get the inputs, convert it to view model, and then convert the view model into outputs, are complicating things too much (although it has its advantages for sure). This is awesome example how it could look like. (also by awesome Kickstarter iOS team).

@acchou
Copy link
Author

acchou commented Feb 8, 2017

Thanks for the comments @sunshinejr. The kickstarter code base is great, and I initially used their "functions for input, observables for output" style. It's definitely a good option. But I still like having Observables as inputs - it just seems more symmetrical and I could see other components written in the same way...

I agree the extra lines for input/output are awkward. The issue with cleaning that up seems to be initializing the inputs. It seems we must choose between:

  1. Statically ensuring all of the inputs are initialized
  2. Having a single data structure for inputs and outputs

I've chosen 1. Most MVVM seem to choose 2. I can see the appeal as it's less rigid to initialize, but it's also an opportunity for mistakes.

@devxoul
Copy link
Owner

devxoul commented Jan 11, 2018

Now RxTodo uses ReactorKit so we can close this issue :)

@devxoul devxoul closed this Jan 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants