Closed johnGachihi closed 4 years ago
@johnGachihi we have.await()
from ktx
so you can just use that one. You can cross check the implementation on other repos too
Nice. I didn't know .await() existed. So doesn't that mean the functions in this file are unnecessary
@johnGachihi we have
.await()
fromktx
so you can just use that one. You can cross check the implementation on other repos too
Is this better: https://gist.github.com/johnGachihi/6590de6e845e2089eca33e53e426bc9d. Or have I made it worse
Hi @johnGachihi I've just gone through the snippet and noticed FRC
. I think using such initials tends to bring more confusion. Names should be as clear as possible so I suggest you change that. Also, why are you giving the WifiDetailsModel
a body. You should create an object
instead and put the extra logic there. The model should contain the fields only.
I have edited it @michaelbukachi https://gist.github.com/johnGachihi/6590de6e845e2089eca33e53e426bc9d
Cool. Looks better.
One last thing. You should rename createUsingFirebaseRemoteConfigData
to just create
since WifiDetailsModelFactory.create(firebaseRemoteConfig)
paints a clear picture of what is going on.
The functions in that file were way long before ktx was added so now they are not in use. We are still doing the cleanup to remove unused code
One question to @michaelbukachi and @johnGachihi . Now that @johnGachihi implemementation exposes the sealed class to the view, should we do a re-write to all the viewmodel layers to follow the same?
I took that approach because it only requires one livedata for all states of data. The current approach requires livedata for each state of data. For example, for organizers data in AboutViewModel
there are
organizersLiveData
for successful fetchorganizersError
for unsuccessful fetchThis means the view has to observe the two livedata to know the state of the organizers data. But since the data can't take more than one state (e.g the fetch can't be both successful and unsuccessful), only one of the two livedata will emit data in the lifetime of the view. The other livedata is observed though it never emits a value. If the data had more states e.g loading then there would be more livedata the view observes though they never emit a value in the view's lifetime
With approach I took the view only needs to listen to one livedata.
But I see FirebaseResult
should be renamed to lack the word Firebase
if it is to be exposed to views.
But one state carries the message and the other carries the result. Since the types are not the same, I think it's okay to have different livedata. The only change I would make is having one LiveData for handling all errors. We cannot use Result
as name since it's already used in Kotlin.
Using one LiveData for all errors will bring exceptions lets say for example we have one error before then try to update the value with another error. Not sure if the behavior is same with LiveData but for MediatorLiveData you cannot do the same
I'm lost :sweat_smile: . Could you share a snippet to demonstrate this?
@wangerekaharun I not sure I understood correctly but is this what you meant:
livedata.setValue(FirebaseResult.Error("error") // Observer will be updated
...
livedata.setValue(FirebaseResult.Error("error") // Observer will not be updated
// since value set is the same as previous one
So for example this Original File has a couple of livedata for error then as to my understanding of having one error for livedata checkout this gist. @michaelbukachi let me know if this is what your were saying
@johnGachihi see in my gist the second setting value for the error livedata will cause an exception. Unless the behaviour changed, cause i tried the same back in early 2018 and was getting exceptions
I can't access the gist. The link is taking me to a 404
I can't access the gist. The link is taking me to a 404
Fixed
@wangerekaharun Yes that's what I meant. Just all exceptions are delivered as strings and delivered to the view for displaying in Snackbar/Toast have multiple liveData just makes a bit repetitive.
@wangerekaharun Yes that's what I meant. Just all exceptions are delivered as strings and delivered to the view for displaying in Snackbar/Toast have multiple liveData just makes a bit repetitive.
Let me do a sample and see if there will be any issues especially on rotation change. There is that exception where lets say we have an error in method 1 and you update again to set value in method 2 when there is also an error, you get this exception
If the given LiveData is already added as a source but with a different Observer, IllegalArgumentException will be thrown.
@johnGachihi You can now commit all changes suggested then we merge the PR
Including the changes on the livedata?
No the others. Will test on the Livedata part
Alright
@johnGachihi You can now commit all changes suggested then we merge the PR
Finally. Now, I can go play some games :laughing: .
About the
WifiDetailsRepoImpl
, is this a better approach: https://gist.github.com/johnGachihi/6590de6e845e2089eca33e53e426bc9d