[Code health] Android Resources in ViewModel is not good practice

[anandwana001]

Original discussion https://github.com/google/ground-android/issues/2519

As we know ViewModel is treated as a layer between UI and other layers. ViewModel consists of all the business logic that is needed for the UI to function as per the requirement.

Now, if we see the class AbstractTaskViewModel, which is a common ViewModel under the tasks feature. One of the features here is showing of error message which is a MutableLiveData and XML is accessing it directly based on it's value.

So far, everything is correct, but now, to get the error string message, we are using the android.content.res.resources which is acting as a ViewModel dependency here, and it is not a good practice to use Android specific objects in ViewModel layer, as it might create a memory leak issue.

Solution

We can shift the String assignment work to the respective Fragment or Activity. Or maybe we can use BindningAdapter.

More Info

Memory Leak

Lifecycle Mismatch: ViewModels are designed to outlive Activity and Fragment lifecycles, meaning they can be retained across configuration changes (e.g., screen rotations). If a Context or Resources object tied to an Activity or Fragment is held by the ViewModel, it can prevent the Activity or Fragment from being garbage collected, leading to memory leaks.

Increased Coupling

Tight Coupling: Passing a Context or Resources directly into a ViewModel increases the coupling between the ViewModel and the Android framework. This makes the ViewModel less reusable and harder to test, as it now depends on Android-specific classes.

Testing Complexity

Difficult to Mock: Unit testing ViewModels becomes more complex if they depend on Android framework classes. This is because you need to mock or provide fake implementations of Context or Resources, which can be cumbersome. Reduced Testability: Ideally, ViewModels should be tested in isolation without relying on Android framework classes. Dependency injection and repository patterns help achieve this by abstracting dependencies.

@sufyanAbbasi @shobhitagarwal1612 WDYT?

[gino-m]

+1 re coupling and testing complexity. One question about memory leaks - if we call getResources() on the application context, wouldn't those always outlive the ViewModels?

I've assigned this as part of your lifecycle fix work to track for later. Thanks for reporting!

[shobhitagarwal1612]

@anandwana001 If you haven't started working on it, can I pick this one up?

[shobhitagarwal1612]

I was able to quickly pull together https://github.com/google/ground-android/pull/2713 which removes this dependency. PTAL