The Seven (Actually 10) Cardinal Sins of Android Development
Merry Christmas to everyone! In this article, I intend to show you a few commonly recurring mistakes that some developers make by accident, that can potentially break your app.
Generally, there are particular patterns that you see in code, and you know that in certain scenarios — generally after triggering process death / low memory condition — that app will crash or fail to work in horrible ways.
Let’s look at a few ways we can make that happen.
1.) Static reference to Activity/Fragment/View
If you see code that looks like this:
public static Activity activity = ...;
public static Fragment fragment = ...;
public static TextView textView = ...;
public static RecyclerView recyclerView = ...;or
companion object {
var activity: Activity? = ...
var fragment: Fragment? = ...
var textView: TextView? = ...
var recyclerView: RecyclerView? = ...
}Chances are, you’re leaking memory, and your views will never die.
Using a static reference to Android-specific OS components or UI components is generally used as a shortcut, so that it “becomes easier to call methods on it from another screen”.
No, don’t do that.
To provide new initial values to a new component, use Intent.putExtra, or Fragment.setArguments.
For communicating between screens, you want to use something like a shared ViewModel, and something like a shared LiveData.
Instead of static refs, one should favor at least some form of event dispatching mechanism — even an EventBus is an improvement. WeakReference is also not a solution.
You could theoretically also use startActivityForResult , but having multiple Activities in one’s own app flow — two Activities on the task stack at once — is already its own can of worms.
2.) Instance reference to Fragment (without using findFragmentByTag)
If you see code that looks like this:
public class MyActivity extends AppCompatActivity {
WeatherFragment weatherFragment = new WeatherFragment();
ChatFragment chatFragment = new ChatFragment();
...
}or
class MyActivity: AppCompatActivity() {
private val weatherFragment = WeatherFragment()
private val chatFragment = ChatFragment()
...
}Then these Fragments will be initialized each time, instead of first trying to check if they already exist via supportFragmentManager.findFragmentByTag().
val fragment = supportFragmentManager
.findFragmentByTag("myFragment")
?: MyFragment().also { fragment ->
addFragment(fragment, "myFragment")
}this.myFragment = fragment
It’s actually okay to have instance references to Fragments, as long as we always first definitely make sure to check if they already exist via findFragmentByTag().
Otherwise, we’ll end up with either duplicate, or accidentally unattached fragments. And if we’re trying to talk to unattached fragments, we’ll get crashes.
3.) List of Fragments inside FragmentPagerAdapter
If you see code that looks like this:
public class FragmentAdapter extends FragmentPagerAdapter {
private final List<Fragment> mFragmentList = new ArrayList<>();
private final List<String> mFragmentTitleList = new ArrayList<>();
public FragmentAdapter(FragmentManager manager) {
super(manager);
}
@Override
public Fragment getItem(int position) {
return mFragmentList.get(position);
}
@Override
public int getCount() {
return mFragmentList.size();
}
public void addFragment(Fragment fragment, String title) {
mFragmentList.add(fragment);
mFragmentTitleList.add(title);
}
@Override
public CharSequence getPageTitle(int position) {
return mFragmentTitleList.get(position);
}
}Chances are, your app will break on process recreation. Why? Because getItem() is never called, the system has already added the Fragments to the FragmentManager.
val fragment = MyFragment()fragment.setController(this) // this is wrongfragmentAdapter.addFragment(fragment) // this is wrong
These Fragments that were added with fragmentAdapter.addFragment() will be ignored entirely. If we assume similarly to #2 that we can safely communicate with fragments we created at our runtime, these fragments will definitely be unattached, and the app will crash.
Fatal Exception: java.lang.IllegalStateException
Fragment has not been attached yetIn simpler cases, the solution is to instantiate the Fragment directly inside the getItem() call.
Fragment instances can be accessed by their tags, following the instructions in this Stack Overflow answer.
For dynamic FragmentPagerAdapters, the solution is a bit trickier, but not impossible.
If you have addFragment and removeFragment in your FragmentPagerAdapter, chances are this is what you intended to write. No, adding fragment instances directly to a FragmentPagerAdapter from outside won’t work reliably.
4.) Fragment with constructor arguments, without using FragmentFactory
If you have code that looks like this:
class MyFragment(
val bookDao: BookDao
): Fragment() {
...
}or
public class MyFragment extends Fragment {
private final BookDao bookDao; public MyFragment(BookDao bookDao) {
this.bookDao = bookDao;
}
}
And you don’t have code that looks like this:
public class FragmentFactoryImpl extends FragmentFactory {
private final BookDao bookDao; public FragmentFactoryImpl(BookDao bookDao) {
this.bookDao = bookDao;
} @NonNull
@Override
public Fragment instantiate(
@NonNull ClassLoader classLoader,
@NonNull String className,
@Nullable Bundle args) { if(className.equals(MyFragment.class.getName())) {
return new MyFragment(bookDao);
} ...
Then your app is going to crash with the following exception:
Unable to instantiate fragment: make sure class name exists, is public, and has an empty constructor that is publicOriginally I intended to write this example with the parameter String title, but sending a dynamic argument would still require using the fragment.setArguments(Bundle), even when using the FragmentFactory, whoops! There is no way around using fragment.setArguments(Bundle) if we want a stable app.
5.) Setting variables on Fragment through setters on creation
If you have code that looks like this:
val dialogFragment = MyDialogFragment()
dialogFragment.onDateSelectedListener = this
dialogFragment.show(supportFragmentManager, "dialog")
Chances are, your dialog won’t have a listener set when the system recreates it.
When parameters are not sent to the Fragment using setArguments(), then these parameters should be set to it again on recreation.
class MyActivity: AppCompatActivity(), DateSelectedListener {
override fun onCreate(savedInstanceState: Bundle?) {
super.onCreate(savedInstanceState) val dialog: DialogFragment? = supportFragmentManager.findFragmentByTag("dialog") if(dialog != null) {
dialog.onDateSelectedListener = this
}
}
}
Of course, if we forget to do this, generally we would just get a DatePickerDialog that doesn’t do anything when the date is selected.
When providing callbacks to a DialogFragment, the proper solution is to use setTargetFragment().
But this same mistake can actually result in crashes, when for example a list of data is passed around in the same way “because implementing Parcelable would have been too much effort”, but the data is not there on recreation.
Chances are, you didn’t need Parcelable, you needed local data persistence.
Another scenario is if one would like to use these setters on the Fragment instance to “inject dependencies”. These dependencies will not be there on recreation, and the app will crash.
6.) Sharing state between fragments using Jetpack ViewModel, without using SavedStateHandle (or equivalent)
When you have code that looks like this:
private val currentFilter = MutableLiveData(TasksFilterType.ALL)fun currentFilter(): LiveData<TasksFilterType> = currentFilter
But you don’t have code that looks like this:
class MyViewModel: ViewModel() {
fun saveState(bundle: Bundle) {
bundle.putSerializable("currentFilter", currentFilter.value!!)
} fun restoreState(bundle: Bundle) {
currentFilter.value =
bundle.getSerializable("currentFilter") as TasksFilterType
}
}class MyActivity: AppCompatActivity() {
override fun onCreate(savedInstanceState: Bundle?) {
super.onCreate(savedInstanceState)
viewModel = ViewModelProviders.of(this)
.get(MyViewModel::class.java, object: ViewModelProvider.Factory {
override fun <T : ViewModel> create(vmClass: Class<T>) =
MyViewModel().also { vm ->
savedInstanceState?.run {
vm.restoreState(getBundle("viewModelState"))
}
}
})
} override fun onSaveInstanceState(bundle: Bundle) {
super.onSaveInstanceState(bundle)
bundle.putBundle("viewModelState", viewModel.saveState(Bundle()) }
}
Or, when using Jetpack to its fullest, you still don’t have code that looks like this:
private val currentFilter = savedStateHandle.getLiveData("currentFilter", TasksFilterType.ALL)Then chances are, your app is causing frustration to end-users when they try to take a picture of their delicious Christmas dinner and then they return to your app!
Technically I’ve already mentioned this before: when using Jetpack, a ViewModel should be created using an AbstractSavedStateViewModelFactory. This way it is possible to obtain a SavingStateLiveData, that will automatically persist/restore the state correctly.
7.) Having any static non-final variables at all without special considerations, anywhere in your app
If you have code that looks like this:
public class DataHolder {
private DataHolder() {} public static List<Data> data = null;
}
or like this:
object DataHolder {
var data: List<Data>? = null
}and on your splash screen, you have something like this:
class SplashActivity: AppCompatActivity() {
...
DataHolder.data = data
startActivity(Intent(this, SecondActivity::class.java))
finish()
...
}and on your second Activity, you have something like this:
class SecondActivity: AppCompatActivity() {
override fun onCreate(savedInstanceState: Bundle?) {
super.onCreate(savedInstanceState)
DataHolder.data!!.let { data ->
...
}
}
}Then your app will crash!
When Android recreates an app, it will restore the last top-most Activity in the task stack. This means that while SplashActivity is the one that would handle the MAIN action, it’s not actually the one with which the app will start! It will be SecondActivity, and the DataHolder will NEVER be initialized.
This sort of data loading would require special considerations, for example exposing a LiveData that would do the initial data load inside LiveData.onActive().
(This LiveData would reside in the ViewModel as you normally expect, the Activity already “broadcasts” its active status by calling observe with the LifecycleOwner, and triggers the LiveData to become active.)
You must never expect an Activity to have previously executed in your app, there is no guarantee for this at all.
(Note: One particular place where static variables can work correctly is variables assigned only once by Application.onCreate().)
8.) Only executing initial data load when savedInstanceState == null
If you have code like this:
class MainActivity: AppCompatActivity() {
override fun onCreate(savedInstanceState: Bundle?) { super.onCreate(savedInstanceState)
...
if(savedInstanceState == null) {
viewModel.startDataLoading()
}
Then your app will NEVER start data loading after it’s recreated.
Not to mention, it’s not the Activity that should know when to start data loading. Same rule applies as above: LiveData.onActive() should be the one to figure it out, the data load should be triggered by observing the result set.
(Note: you can also ensure that the data loading is invoked at least once by triggering the data load in the ViewModel constructor, although that would not defer and execute it only as an effect of onStart. It can work correctly for certain scenarios when mirrored with the other ViewModel lifecycle callback, onCleared()— which works correctly if you use the ViewModelProvider as per docs).
9.) Executing the initial fragment transaction outside of an if(savedInstanceState == null) block
If you have code that looks like this:
class MainActivity: AppCompatActivity() {
override fun onCreate(savedInstanceState: Bundle?) {
super.onCreate(savedInstanceState) supportFragmentManager.beginTransaction()
.replace(R.id.container, FirstFragment())
.commit()
}
and you don’t have code that looks like this:
class MainActivity: AppCompatActivity() {
override fun onCreate(savedInstanceState: Bundle?) {
super.onCreate(savedInstanceState)
if(savedInstanceState == null) {
supportFragmentManager.beginTransaction()
.add(R.id.container, FirstFragment())
.commit()
}Then you are overwriting the system-recreated Fragments, and all navigation history will be lost!
In fact, if you use addToBackStack(), then there’s minimal guarantee what your actually restored navigation history will be.
And if the first snippet had used add(), we’d end up with multiple overlapping fragments. No, setting a clickable full-screen background with a background color to hide overlapping fragments isn’t a real solution!
10.) Using onResume/onPause for anything other than enabling navigation actions or retrieving the camera
This is not actually a process death bug, but it’s still worth a mention.
If you have code that looks like this:
override fun onResume() {
super.onResume()
viewModel.events.observe(this /*viewLifecycleOwner*/, Observer {
// ...
})
}Or you have code that looks like this:
override fun onPause() {
super.onPause()
compositeDisposable.clear()
}Chances are, your app is quite frustrating to use, and could potentially have some hidden bugs!
onResume can run multiple times: put the app in background a few times and bring it foreground, it will run each time. If you register observers in onResume, but those observers are only removed in onDestroy, then your observer will run many many times!
If you stop observing events of an event bus in onPause, then by having a DialogFragment, an IntentChooser, or a runtime permission request dialog over your app, you suddenly start to ignore events. If these events are not enqueued, this can result in cryptic bugs (“why does my code not run?”).
If a user expects to use multi-window mode on their phone, it is very frustrating that putting the other app in focus suddenly makes the app “no longer work until tapped again”. Therefore, moving the clear() in onStop() is a better bet: this is why LiveData becomes inactive inside onStop() and not onPause().
While Android 10 already has support for multi-resume (all apps in multi-window are resumed), Android 10 is not as common yet.
Speaking of which, if you are writing a camera app, you might want to also handle the new lifecycle callback onTopResumedActivityChanged, so that it works correctly on Android 10.
Conclusion
Hopefully that helped you learn how to identify patterns in an Android codebase that can cause nasty crashes in production, and grumpy users. The best gift is an app with 99.9%+ crash-free rates. 🎁
Happy bug hunting, and Merry Christmas! 🎄🎉😊
The Reddit discussion thread is at https://www.reddit.com/r/androiddev/comments/eezxux/the_seven_actually_10_cardinal_sins_of_android/ .
