search

nhphong / nimble-surveys

0 stars 0 forks source link

The .let{ } scope looks hacky #6

Closed sleepylee closed 5 years ago

sleepylee commented 5 years ago

From what I observe here, instead of disposing the Disposable object properly, this solution appears to be quite hacky to me...how do you think?

nhphong commented 5 years ago

Like what I commented there, we don't need to unsubscribe from the observable because it only modifies value of a LiveData object. LiveData object is life-cycle aware so it never emits value to Activity/Fragment when they're in the paused state.

Actually, the Android Architecture Components are quite new and I'm still in the process of learning it. So I took this Code Challenge as a chance to experiment it. I hope my understanding about it is correct. 😄

sleepylee commented 5 years ago

well...the LiveData is ok, but the Disposable created from your subscribe() is not going anywhere yet, hence side effect could happen when we don't expect them i.e when closing an Activity. I believed Rx has documented it quite well on how and when should we dispose them. Again...this comes to the root concern of my first question...mixing these 2 together ain't gonna be easy for handling them gracefully

nhphong commented 5 years ago

Yes, I totally agree on that. We should find a better way to handle them more gracefully.

For your information, I was inspired by the following Google sample code to make this challenge https://github.com/googlesamples/android-architecture/tree/todo-mvvm-live-kotlin/ (It's using callbacks/listeners, not RxJava)

sleepylee commented 5 years ago

I'd give you a hint regarding this: https://stackoverflow.com/a/47699547/5315499 😉(please note: it's not the solution, just the idea of what I'm mentioning to)

nhphong commented 5 years ago

I'm still wondering about it. Since SurveysRepository only works with Single and Completable (which is not an infinite stream of data/events). Shall it dispose itself after completing/emitting the value? 🤔 🤔 🤔

nhphong commented 5 years ago

Implementing the disposal is easy. But i'm still concerning whether we need to do that. 😄

sleepylee commented 5 years ago

imo, the side effect it could cause when the activity/context is not intended to be visible anymore should be considered, for this case, it won't affect much because your background work is just to update the db with the coming data. But side effect never means to always be good...it's troublesome when it comes to debugging, instead, if the result is not supposed to be there, we shouldn't keep it or further process to save the resource, opinionated 😀

nhphong commented 5 years ago

Agree 👍 I will update the code

nhphong commented 5 years ago

@sleepylee Check it out https://github.com/nhphong/nimble-surveys/pull/13