Common mistakes when using Architecture Components
Android, ehh, Android…. development is what is always described as one of the painful things, but we still love it tho, of course there’s a reason why Android’s messy, but have you seen front-end development?
Everything was peaceful untill Fragments were introduced with their own lifecycle and to make things messier, Google decided to make things even more messier, Fragment’s view will have it’s own lifecycle.
Nevertheless Fragment’s nowadays are matured and more stable than the ones 2 years ago.
And Google also released architecture components to ease our lives, or did they?
There are several mistakes you can make when using architecture components, even if you don’t make them, you should be aware.
This article assumes you have knowledge what ViewModel is, SOLID principles and some general knowledge how things make sense (at least in this article).
Leaking LiveData observers in Fragments
1
2
3
4
5
6
override fun onViewCreated(view: View, savedInstanceState: Bundle?) {
super.onViewCreated(view, savedInstanceState)
viewModel.liveData.observe(this){ updateUI(it) } // this means the Fragment as a LifecycleOwner
}
Google’s lint forces you to use getViewLifecycleOwner(), because if you’re using the Fragment as a LifecycleOwner you’re always adding observers and never clearing them because the LifecycleOwner didn’t reach the DESTROYed state.
🚫 Don’t use Fragment as a LifecycleOwner when observing Live Data.
✅ Use the getter for viewLifecycleOwner, since your observer has a DESTROYed state when it reaches onDestroyView.
Observing flows in launchWhenX
Coroutines StateFlow and SharedFlow, were pushed as a replacement for LiveData but there wasn’t something that’ll clear up the references when using launchWhenX
You decided to get rid of LiveData and you started using viewLifeCycleOwner.lifecycleScope.launchWhenResumed or viewLifeCycleOwner.lifecycleScope.launchWhenStarted to ensure that a given coroutine runs only in that specific lifecycle state, there’s a deep dive article worth reading why that’s the thing you shouldn’t use.
TL:DR; flows can be dropped when config changes, every time you call this function LifecycleController and PausingDispatcher are created and also launchWhenX functions also use Dispatchers.Main.immediate
🚫 Don’t use launchWhenX.
✅ Use the new repeatOnLifecycle or just use flow.asLiveData.
Thanks to Manuel Vivo who provided a great explanation.
Reloading data after every rotation and creation of the view in the Fragment
How many PRs I have rejected because of this thing I lost count.
It comes in cancerous form like
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
@HiltViewModel
class DetailedMovieViewModel @Inject constructor(
private val moviesRepository: MoviesRepo,
private val savedStateHandle: SavedStateHandle
) : ViewModel() {
private val args = DetailedMovieFragmentArgs.fromSavedStateHandle(savedStateHandle)
private val movieID get() = args.movieId
private val movieData = retrofitStateInitialLoading<DetailedMovieModel>()
val movie = movieData.asStateFlow()
fun fetchMovieData() {
viewModelScope.launch {
movieData.value = withContext(Dispatchers.IO){
moviesRepository.getDetailedMovie(movieID)
}
}
}
}
then we head over where my head hurts the most
1
2
3
4
5
6
7
8
9
10
11
@AndroidEntryPoint
class DetailedMovieFragment : Fragment(R.layout.fragment_detailed_movie){
private val viewModel by viewModels<DetailedMovieViewModel>
override fun onViewCreated(view: View, savedInstanceState: Bundle?) {
super.onViewCreated(view, savedInstanceState)
viewModel.fetchMovieData() // <----- this line
}
}
How do we fix this?
Simple.
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
@HiltViewModel
class DetailedMovieViewModel @Inject constructor(
private val moviesRepository: MoviesRepo,
private val savedStateHandle: SavedStateHandle
) : ViewModel() {
private val args = DetailedMovieFragmentArgs.fromSavedStateHandle(savedStateHandle)
private val movieID get() = args.movieId
private val movieData = retrofitStateInitialLoading<DetailedMovieModel>()
val movie = movieData.asStateFlow()
init {
fetchMovieData()
}
private fun fetchMovieData() {
viewModelScope.launch {
movieData.value = withContext(Dispatchers.IO){
moviesRepository.getDetailedMovie(movieID)
}
}
}
}
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
@AndroidEntryPoint
class DetailedMovieFragment : Fragment(R.layout.fragment_detailed_movie){
private val viewModel by viewModels<DetailedMovieViewModel>
override fun onViewCreated(view: View, savedInstanceState: Bundle?) {
super.onViewCreated(view, savedInstanceState)
viewLifecycleOwner.lifecycleScope.launch {
repeatOnLifecycle(Lifecycle.State.STARTED) {
viewModel.movie.collect { movie ->
updateUI(movie)
}
}
}
}
}
🚫 Don’t call the function to fetch the data in onViewCreated nor in onCreateView nor anywhere inside the Fragment unless it’s an action that’s triggered by a user input or action.
✅ Use the init function inside the ViewModel, that’s called only once when the ViewModel is created.
Misusing data holders
MutableLiveData and LiveData are data holders, there are several mistakes you can make.
Data holders as variables
1
var data : MutableLiveData<DetailedMovieModel> = MutableLiveData()
1
var data : MutableStateFlow<DetailedMovieModel?> = MutableStateFlow(null)
🚫 Don’t use var as a way to create a data holder.
✅ Use val.
You’re creating unnecessary getter and setter for the data holder, if MutableLiveData was a variable you could’ve just assigned the value to it directly without using .value or postValue().
Exposing mutable data holder instead of immutable
You had your data holder fixed but
1
val data : MutableLiveData<DetailedMovieModel> = MutableLiveData()
1
val data : MutableStateFlow<DetailedMovieModel?> = MutableStateFlow(null)
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
class MovieFragment : Fragment(){
private val viewModel by viewModels<DetailedMovieViewModel>
override fun onViewCreated(view: View, savedInstanceState: Bundle?) {
super.onViewCreated(view, savedInstanceState)
viewModel.data.observe....
}
private fun doAMistake(){
viewModel.data.value = someMistake
}
}
I’ve seen code like this and it makes my yesterday’s meal turn over.
1
2
private val movieData : MutableLiveData<DetailedMovieModel> = MutableLiveData()
val movie : LiveData<DetailedMovieModel> = movieData
1
2
private val movieData : MutableStateFlow<DetailedMovieModel?> = MutableStateFlow(null)
val movie = movieData.asStateFlow()
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
class MovieFragment : Fragment(){
private val viewModel by viewModels<DetailedMovieViewModel>
override fun onViewCreated(view: View, savedInstanceState: Bundle?) {
super.onViewCreated(view, savedInstanceState)
viewModel.movie.observe....
}
private fun doAMistake(){
viewModel.movie.value = someMistake //forbidden because it's immutable and you can't do anything
}
}
🚫 Don’t expose Mutable data holder to the Fragment, if you’re using two-way binding with data-binding, then stop using data-binding and use view-binding, this is a harsh and direct statement but just don’t use data-binding, if data-binding was a great solution there wouldn’t be a code pollution and view-binding wouldn’t have appeared as a replacement, data-binding also breaks SOLID.
✅ Expose an immutable data holder.
Using getters for the immutable live data holder
1
2
private val movieData : MutableLiveData<DetailedMovieModel> = MutableLiveData()
val movie : LiveData<DetailedMovieModel> get() = movieData
1
2
private val movieData : MutableStateFlow<DetailedMovieModel?> = MutableStateFlow(null)
val movie get() = movieData.asStateFlow()
Kotlin docs just for the rescue
If you define a custom getter, it will be called every time you access the property (this way you can implement a computed property)
🚫 Don’t use a getter for the immutable data holder.
✅ Initialize it immediately after the mutable data holder.
1
2
private val movieData : MutableLiveData<DetailedMovieModel> = MutableLiveData()
val movie : LiveData<DetailedMovieModel> = movieData
1
2
private val movieData : MutableStateFlow<DetailedMovieModel?> = MutableStateFlow(null)
val movie = movieData.asStateFlow()
Leaking ViewModels
It is clearly highlighted that we shouldn’t be passing View references to ViewModel, that’s clear, otherwise you’re just killing the purpose of the ViewModel.
Forgetting to unsubscribe a listener
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
@Singleton
class LocationRepository @Inject constructor() {
private var localListener: ((Location) -> Unit)? = null
fun setOnLocationChangedListener(listener: (Location) -> Unit) {
localListener = listener
}
fun removeOnLocationChangedListener() {
localListener = null
}
private fun onLocationUpdated(location: Location) {
listener?.invoke(location)
}
}
@HiltViewModel
class MapViewModel @Inject constructor(
private val repository : LocationRepository
): ViewModel() {
private val locationData = MutableStateFlow<LocationRepository.Location?>(null)
init {
repository.setOnLocationChangedListener {
locationData.value = it
}
}
}
In this sample we’re not clearing the reference and we can’t make the listener garbage collectable.
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
@HiltViewModel
class MapViewModel @Inject constructor(
private val repository : LocationRepository
): ViewModel() {
private val locationData = MutableStateFlow<LocationRepository.Location?>(null)
init {
repository.setOnLocationChangedListener {
locationData.value = it
}
}
override onCleared() {
repository.removeOnLocationChangedListener()
}
}
🚫 Don’t just create references that’ll be held in the ViewModel.
✅ Every reference has to be cleared in onCleared().
Observing inside a ViewModel
You know about NOT referencing Views inside a ViewModel DO NOT DO OBSERVE ANYTHING INSIDE A ViewModel AS WELL!
ViewModel objects are designed to outlive specific instantiations of views or LifecycleOwners. This design also means you can write tests to cover a ViewModel more easily as it doesn’t know about view and Lifecycle objects. ViewModel objects can contain LifecycleObservers, such as LiveData objects. However ViewModel objects must never observe changes to lifecycle-aware observables, such as LiveData objects. If the ViewModel needs the Application context, for example to find a system service, it can extend the AndroidViewModel class and have a constructor that receives the Application in the constructor, since Application class extends Context.
One day I opened the proton-mail-android app and glanced over EditContactDetailsViewModel
🚫 Don’t ever do this.
✅ Everything you need to observe that tackles UI element should be in a layer where the UI is Fragment or an Activity.
Observing in the wrong places
🚫 Do not observe inside onResume .
🚫 Do not observe inside onCreate in the Fragment.
🚫 Do not observe inside a Service unless you manage it’s lifecycle from CREATED > DESTROYED, same goes for a BroadcastReceiver.
Resetting the current observer
1
2
3
4
fun <T> LiveData<T>.resetObserver(owner: LifecycleOwner, observer: Observer<T>) {
removeObserver(observer)
observe(owner, observer)
}
An observer is unsubscribed before an identical one is subscribed hopefully in onViewCreated by removing and adding the same exact observer you will reset the state so that LiveData will deliver the latest result again when onStart() happens.
🚫 Do not do this.
The final boss, MutableLiveData for single shot events
Sorry Proton for bashing your code base, again I glanced over at MailboxViewModel
1
2
3
4
5
private val _hasSuccessfullyDeletedMessages = MutableLiveData<Boolean>()
val hasSuccessfullyDeletedMessages: LiveData<Boolean>
get() = _hasSuccessfullyDeletedMessages
in the Activity
1
2
3
4
5
6
7
8
9
mailboxViewModel.hasSuccessfullyDeletedMessages.observe(
this,
{ isSuccess ->
//Timber.v("Delete message status is success $isSuccess")
if (!isSuccess) {
showToast(R.string.message_deleted_error)
}
}
)
After some time Google came up with a hacky solution to use a nightmare called Event
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
class Event<out T>(private val content: T) {
private val hasBeenHandled = AtomicBoolean(false)
/**
* Returns the content and prevents its use again.
*/
fun getContentIfNotHandled(): T? {
return if (!hasBeenHandled.getAndSet(true)) {
content
} else {
null
}
}
/**
* Returns the content, even if it's already been handled.
*/
fun peekContent(): T = content
}
In order to use the event, you need to getContentIfNotHandled() first in order to see if the content is consumed, I just deleted that thing and went to use a Channel.
Keep it simple, stupid.
🚫 Do not use MutableLiveData or flow to show a Toast a SnackBar or even worse a Dialog or something that’ll just spam the user endlessly.
✅ Use a Channel or a SharedFlow or the hacky solution Google provided and shoot yourself in the foot with writing boilerplate code.
This post is more like a rant, hope your ego isn’t hurt so far and you learnt to make the code a better place.
Thanks for reading and not getting hurt (hopefully).