8

I need some opinions on Rx and MVVM. Its being done in iOS, but I think its fairly general programming question.

The small team I joined is using Rx (I've never used it before) and I'm trying to learn and catch up to them. Looking at the code, I think there are thousands of lines of over-engineered code that could be done so much simpler. From a non Rx point of view, I think we are following some bad practises, from an Rx point of view the guys are saying this is what Rx needs to be. I'm trying to discuss this with them, but they are shooting me down saying I just don't know enough about Rx. Maybe thats true, maybe I just don't get it, but they aren't exactly explaining it, just telling me i'm wrong and they are right. I need another set of eyes on this to see if it is just me.

One of the main points is that there are many places where network errors shouldn't complete the observable (i.e. can't call onError), I understand this concept. I read a response from the RxSwift maintainers that said the way to handle this was to wrap your response type in a class with a generic type (e.g. Result<T>) that contained a property to denote a success or error and maybe an error message. This way errors (such as incorrect password) won't cause it to complete, everything goes through onNext and users can retry / go again, makes sense.

The guys are saying that this breaks Rx principals and MVVM. Instead we need separate observables for every type of response. So we have viewModels that contain:

- isSuccessObservable
- isErrorObservable
- isLoadingObservable
- isRefreshingObservable
- etc. (some have close to 10 different observables)

To me this is overkill to have so many streams all frequently only ever delivering 1 or none messages. I would have aimed for 1 observable, that returns an object holding properties for each of these things, and sending several messages. Is that not what streams are suppose to do? Then the local code can use filters as part of the subscriptions. The major benefit of having 1 is that it becomes easier to make it generic and abstract away, which brings us to point 2.

Currently, due to each viewModel having different numbers of observables and methods of different names (but effectively doing the same thing) the guys create a new custom protocol (equivalent of a java interface) for each viewModel with its N observables. The viewModel creates local variables of PublishSubject, BehavorSubject, Driver etc. Then it implements the procotol / interface and casts all the local's back as observables. e.g.

protocol CarViewModelType {
isSuccessObservable: Observable<Car>
isErrorObservable: Observable<String>
isLoadingObservable: Observable<Void>
}

class CarViewModel {
isSuccessSubject: PublishSubject<Car>
isErrorSubject: PublishSubject<String>
isLoadingSubject: PublishSubject<Void>

// other stuff
}

extension CarViewModel: CarViewModelType {
isSuccessObservable {
return isSuccessSubject.asObservable()
}
isErrorObservable {
return isSuccessSubject.asObservable()
}
isLoadingObservable {
return isSuccessSubject.asObservable()
}
}

This has to be created by hand, for every viewModel, of which there is one for every screen and there is 40+ screens. This same structure is copy / pasted into every viewModel. As mentioned above I would like to make this all generic. Have a generic protocol for all viewModels to define 1 Observable, 1 local variable of generic type and handle the cast back automatically. The method to trigger all the business logic could also have its name standardised ("load", "fetch", "processData" etc.). Maybe we could also figure out a few other bits too. This would remove a lot of code, as well as making the code more readable (less messy), and make unit testing much easier. While it could never do everything automatically we could test the basic responses of each viewModel and have at least some testing done by default and not have everything be very boilerplate-y and copy / paste nature.

The guys think that subscribing to isSuccess and / or isError is perfect Rx + MVVM. But for some reason subscribing to status.filter(success) or status.filter(!success) is a sin of unimaginable proportions. Also the idea of multiple buttons and events all "reacting" to the same method named e.g. "load", is bad Rx (why if they all need to do the same thing?)

My thoughts on this are:

- To me its indentical in meaning and architecture, one way is just significantly less code.
- Lets say I agree its not textbook, is it not worth bending the rules to reduce code.
- We are already breaking the rules of MVVM to introduce coordinators (which I hate, as they are adding even more unnecessary code), so why is breaking it to reduce code such a no no.

Any thoughts on the above? Am I way off the mark or is this classic Rx?

Comments
  • 2
    Slightly unrelated to the above, they are also refusing to use any dependency injection framework (saying they are all crap) and hate singletons, so we have this issue of everything having to be passed around from one class to the next. Frequently there are classes taking in and passing around instances that they do nothing with, they simply do it to pass it to somewhere else. Again, all i'm seeing is classes riddled with variables, and constructors with 6 inputs, of which 1 gets used in the class and so on. I find it hard to read, hard to follow and hard to maintain.
  • 2
    DI: Dagger 2 is hard to master but has helped my code be a lot more readable/maintainable and FASTER (in Android)

    Observables: The way I handle network is I have 1 class (retrofit again Android sorry I don't iOS) that JUST handles the call and returns NetEvent<MyResponse> (MyResponse is the translated response not the http response and net event holds error state and message as well so this will always return something) and then handle the response where I need to. Having a million different observables for this is not only overkill it's bad design / not scalable and bad for performance and readability.
  • 3
    Don't become them. Do you. Write what you think is correct. Don't ask them beforehand. Whenever you ask ppl like that they are going to say no cause they "know better".
    What I do is write on my boilerplate app. Test it. And then bring it to work. Again without asking. They can deal with it on the review if they don't like it and EXPLAIN to me what AND why is wrong.

    *The opinions expressed here are my own and I am not responsible for the results - nuclear war etc.!
  • 1
    The project I currently work on is also RxSwift,RxDataSources,etc...

    You are completly right, this is not a good practice.

    First off all: protocol CarViewModelType {

    isSuccessObservable: Observable<Car>

    isErrorObservable: Observable<String>

    isLoadingObservable: Observable<Void>

    }

    Why would you ever need those?

    isSuccess can be done using the onCompleted or the onNext... The isLoading I don't understand why that one is needed. If you have multiple observables you can just concat/combine them until they are done loading a.k.a. isSucces a.k.a. the onNext.

    The isError I really don't get, as this should be the build in onError?!

    About that generic WTF how are they still alive?

    Yes, fix that with a generic protocol or whatever!!!

    About that filter: I don't understand with more context.

    About that load: You are completly right it is the correct way to have multiple elements react to a single observable
  • 0
    I'm not sure what you mean by reacting to the same method, I assume you mean reacting to the same observable.

    Multiple elements reacting to the same observable is completly fine. I don't know why that would be a problem at all. Each element having a single observable smells (very likely to be just plain bad code).

    Identical code : You are completly right.

    You are not breaking the rules.
  • 1
    Hope it helps
  • 0
    Hope it helps
  • 0
    @Rick-C137 Oh nuclear war is coming no matter how hard I try to avoid it. I won't hold you to that haha.

    Currently i'm setting up a team in a different country to them and I have to play nice until we can find the staff over here. Its 3 of them versus 1 of me. All the managers and product owners etc are all over there, its very easy for them to convince others that i'm wrong. They are also not shy about leaving my PR's open and refusing to merge if they don't like it.

    They have also been working on this for a few months before I joined, and with deadlines coming they are very unwilling to change anything. They have agreed with small pieces of what i've said, such as needing generics, but they think anyway of trying to bring them in is a bad approach.

    So for now I'm quietly playing around with doing it "correctly" somewhere else. The war can start when my side gets to take control of something.
  • 0
    @Rick-C137 also, yes i'm familiar with Dagger, the Android team here use it. Thats exactly what I said to them, I want to use something like what the Android guys are doing to avoid having a million variables everywhere. But apparently every approach for doing that in iOS is crap.
  • 0
    @FranklySimple Thanks for the help / opinions. I'M NOT INSANE!!!!!

    ... now I can appreciate Rx haha. To answer some of your questions:

    - They want separate error observables, because the method call (lets call it "login") doesn't return a new observable, it sends onNext events to a observable sitting as a property of the class that contains the method. They don't want the observable to complete when an error message is fired, because most of the error messages are expected (e.g. wrong password). They want the observable to stay open and keep receiving messages, incase the user tries again. But as i've said, the RxSwift maintainers recommend wrapping your response type in an Object that contains this info, rather than multiple observables.
  • 0
    @FranklySimple

    - The calling different methods thing: Keeping the same approach as above in mind. One view model has a "login" method, the next VM has a "getCars" the next a "getMileage" etc. Some of them have multiple such as "updateWithLoading", "updateWithRefresh". What I said was, going down the generic route, we could simply give each VM one method that fetches / processes data and call it the same thing every time. So rather loginVM.login(...), carVM.getCars(...) etc. it would be loginVM.fetch(...), carVM.fetch(...) and so on (name to be decided). This would help the generic approach and share code.
  • 0
    @FranklySimple

    Their problem: It is apparently against Rx / MVVM principals to have multiple different buttons or system events "reacting" to the same method name. So for example the system fires a message to say the screen loaded, we also have a refresh button. Both of these can not call a "fetch" method, because the loading event needs to react to a loading method and the refresh button, needs to be reacting to a refresh method. I'm not sure if you can understand that, because I don't really.
  • 0
  • 1
    @practiseSafeHex

    Oh that's what you meant: oh yes please do, don't be an idiot to not do that wtf
  • 1
    @practiseSafeHex I think they don't understand rx very well. I can very well understand that, after nearly 2 years I'm still finding things in my code that I'm not doing correctly. Refactoring is very important in Rx IMO.

    Anyway, just because the implementation might be a bit more complicated (perhaps they don't know how to make it) than having two methods that do the same thing (that have different UI implementations).
  • 1
    @practiseSafeHex oh and btw for handling states I usually make a utility class that has a PublishSubject that simply emits the state.
    So basically a wrapper for the network call that let's you also subscribe to the current state while also giving you room for mapping network responses to whatever model you actually want.

    I am assuming the same RX stuff exists in iOS so it would be an enum class being emitted: stateEmitter.onNext(LOADING)

    If you want to check any of these out on Android check my boilerplate under "antonis179" on GitHub.

    All the best hope I helped :)
Add Comment