“Don’ts” in modern Android/Kotlin Development (Bad practices & Anti-patterns) — Chapter I
This series of articles will discuss variety of practices and actions that are likely to decrease your: code quality, product stability, relationships with fellow devs and happiness of your PM/client. I will refer to them as “Dont’s”.
Following “Dont’s” I developed throughout my 4 years journey as Android developer. They will concern various different topics; coding bad practices, anti-patterns that might injury code quality.
I also write on “Do’s”, good practices & tips:
Part 1: https://medium.com/@a.poplawski96/dos-in-modern-android-kotlin-development-tips-good-practices-chapter-i-5dfdd276dd6b
Part 2: https://medium.com/@a.poplawski96/dos-in-modern-android-kotlin-development-tips-good-practices-chapter-ii-81b0c0c56667
1. Putting too much responsibility in View Models
View models are meant to be used at a screen level. Single screen can display data, transform displayed data, handle different user interactions, handle input, etc. In other words, single screen can hold multiple functionalities. Since view models represent the whole screen, they are often destined to be large and complex.
That being said, it is important to try keeping complexity of our view models as low as possible. Let’s consider the following example:
class HeavyWeightViewModel(
private val simpleRepository: SimpleRepository,
private val userRepository: UserRepository,
private val analyticsLogger: AnalyticsLogger,
) : ViewModel() {
companion object {
private const val ANALYTICS_EVENT_LOAD_DATA = "event_load_data"
private const val ANALYTICS_PARAM_USER_ID = "param_user_id"
private const val ANALYTICS_PARAM_SCREEN_NAME = "screen_name"
private const val ANALYTICS_EVENT_LOAD_DATA_ERROR = "event_load_data_error"
}
sealed interface ViewState {
object Idle : ViewState
object Error : ViewState
data class DataLoaded(val someData: SomeData) : ViewState
}
private val _viewState = MutableStateFlow<ViewState>(ViewState.Idle)
val viewState = _viewState.asStateFlow()
fun loadData() {
try {
viewModelScope.launch {
withContext(Dispatchers.IO) {
val result = simpleRepository.getSomeData()
withContext(Dispatchers.Main) {
_viewState.value = ViewState.DataLoaded(result)
}
analyticsLogger.logEvent(
eventName = ANALYTICS_EVENT_LOAD_DATA,
parameters = mapOf(
ANALYTICS_PARAM_USER_ID to userRepository.getUserId(),
ANALYTICS_PARAM_SCREEN_NAME to "some_screen",
)
)
}
}
} catch (e: Exception) {
_viewState.value = ViewState.Error
analyticsLogger.logEvent(
eventName = ANALYTICS_EVENT_LOAD_DATA_ERROR,
parameters = mapOf(
ANALYTICS_PARAM_USER_ID to userRepository.getUserId(),
ANALYTICS_PARAM_SCREEN_NAME to "some_screen",
)
)
}
}
// Some more functions
// ...
}For demonstration purposes, it’s a simple class. However in real life scenario and commercial projects, you will often deal with more functions with way more logic.
The best way to unweight view models, is to delegate some of the responsibilities to outer layers.
Usually, Android projects will contain classes from at least two more layers, for example: Repositories (data layer), Interactors/UseCases (domain layer). Let’s utilize them all!
While domain layer is suggested only as optional by official Android architecture recommendations, it’s recommended for medium and large sized projects. Let’s introduce it then for the sake of this example.
I. Delegate responsibilities to outer layers
As said before, the best way to unweight view models, is to delegate some of the responsibilities to outer layers. What can be delegated then?
- Error handling with try/catch blocks
- Dispatchers switching
Let’s introduce GetSomeDataUseCase class and delegate those responsibilities.
class GetSomeDataUseCase(
private val simpleRepository: SimpleRepository,
private val ioDispatcher: CoroutineDispatcher,
) {
sealed interface Result {
data class Success(val data: SomeData) : Result
object Error : Result
}
suspend fun execute(): Result = withContext(ioDispatcher) {
return@withContext try {
Result.Success(simpleRepository.getSomeData())
} catch (e: Exception) {
Result.Error
}
}
}And now, we can replace SimpleRepository with GetSomeDataUseCase in our ViewModel.
class MiddleWeightViewModel(
private val getSomeDataUseCase: GetSomeDataUseCase,
private val userRepository: UserRepository,
private val analyticsLogger: AnalyticsLogger,
) : ViewModel() {
companion object {
private const val ANALYTICS_EVENT_LOAD_DATA = "event_load_data"
private const val ANALYTICS_EVENT_LOAD_DATA_ERROR = "event_load_data_error"
private const val ANALYTICS_PARAM_USER_ID = "param_user_id"
private const val ANALYTICS_PARAM_SCREEN_NAME = "screen_name"
}
sealed interface ViewState {
object Idle : ViewState
object Error : ViewState
data class DataLoaded(val someData: SomeData) : ViewState
}
private val _viewState = MutableStateFlow<ViewState>(ViewState.Idle)
val viewState = _viewState.asStateFlow()
fun loadData() {
viewModelScope.launch {
when(val result = getSomeDataUseCase.execute()) {
is GetSomeDataUseCase.Result.Success -> {
_viewState.value = ViewState.DataLoaded(result.data)
analyticsLogger.logEvent(
eventName = ANALYTICS_EVENT_LOAD_DATA,
parameters = mapOf(
ANALYTICS_PARAM_USER_ID to userRepository.getUserId(),
ANALYTICS_PARAM_SCREEN_NAME to "some_screen",
)
)
}
is GetSomeDataUseCase.Result.Error -> {
_viewState.value = ViewState.Error
analyticsLogger.logEvent(
eventName = ANALYTICS_EVENT_LOAD_DATA_ERROR,
parameters = mapOf(
ANALYTICS_PARAM_USER_ID to userRepository.getUserId(),
ANALYTICS_PARAM_SCREEN_NAME to "some_screen",
)
)
}
}
}
}
}Much better now, since view model doesn’t need to care about handling exceptions and switching dispatchers. View model is now MiddleWeightViewModel. But let’s go one step further.
While we could also delegate analytics tracking to the new use case class that we just created, from my experience, it is often convenient to keep it in the view model. Let’s see how we can make it more readable.
II. Introduce Facade pattern
Facade is a structural design pattern that provides a simplified interface to a library, a framework, or any other complex set of classes.
That’s a general definition. In other words, Facade is about hiding implementation details from client classes.
We can create a Facade for analytics logging.
class ViewModelSpecificAnalyticsLoggerFacade(
private val analyticsLogger: AnalyticsLogger,
private val userRepository: UserRepository,
) {
companion object {
private const val ANALYTICS_SCREEN_NAME = "some_screen"
private const val ANALYTICS_EVENT_LOAD_DATA = "event_load_data"
private const val ANALYTICS_PARAM_USER_ID = "param_user_id"
private const val ANALYTICS_PARAM_SCREEN_NAME = "screen_name"
private const val ANALYTICS_EVENT_LOAD_DATA_ERROR = "event_load_data_error"
}
fun logDataLoadSuccess() {
analyticsLogger.logEvent(
eventName = ANALYTICS_EVENT_LOAD_DATA,
parameters = mapOf(
ANALYTICS_PARAM_USER_ID to userRepository.getUserId(),
ANALYTICS_PARAM_SCREEN_NAME to ANALYTICS_SCREEN_NAME,
)
)
}
fun logDataLoadError() {
analyticsLogger.logEvent(
eventName = ANALYTICS_EVENT_LOAD_DATA_ERROR,
parameters = mapOf(
ANALYTICS_PARAM_USER_ID to userRepository.getUserId(),
ANALYTICS_PARAM_SCREEN_NAME to ANALYTICS_SCREEN_NAME,
)
)
}
}Lovely, we were able to hide implementation details and encapsulate constants.
Now, let’s use it in the view model and compare it with the HeavyWeightViewModel that we started with.
// After
class LightWeightViewModel(
private val getSomeDataUseCase: GetSomeDataUseCase,
private val analyticsLoggerFacade: ViewModelSpecificAnalyticsLoggerFacade,
) : ViewModel() {
sealed interface ViewState {
object Idle : ViewState
object Error : ViewState
data class DataLoaded(val someData: SomeData) : ViewState
}
private val _viewState = MutableStateFlow<ViewState>(ViewState.Idle)
val viewState = _viewState.asStateFlow()
fun loadData() {
viewModelScope.launch {
when(val result = getSomeDataUseCase.execute()) {
is GetSomeDataUseCase.Result.Success -> {
_viewState.value = ViewState.DataLoaded(result.data)
analyticsLoggerFacade.logDataLoadSuccess()
}
is GetSomeDataUseCase.Result.Error -> {
_viewState.value = ViewState.Error
analyticsLoggerFacade.logDataLoadError()
}
}
}
}
}
// Before
class HeavyWeightViewModel(
private val simpleRepository: SimpleRepository,
private val userRepository: UserRepository,
private val analyticsLogger: AnalyticsLogger,
) : ViewModel() {
companion object {
private const val ANALYTICS_EVENT_LOAD_DATA = "event_load_data"
private const val ANALYTICS_PARAM_USER_ID = "param_user_id"
private const val ANALYTICS_PARAM_SCREEN_NAME = "screen_name"
private const val ANALYTICS_EVENT_LOAD_DATA_ERROR = "event_load_data_error"
}
sealed interface ViewState {
object Idle : ViewState
object Error : ViewState
data class DataLoaded(val someData: SomeData) : ViewState
}
private val _viewState = MutableStateFlow<ViewState>(ViewState.Idle)
val viewState = _viewState.asStateFlow()
fun loadData() {
try {
viewModelScope.launch {
withContext(Dispatchers.IO) {
val result = simpleRepository.getSomeData()
withContext(Dispatchers.Main) {
_viewState.value = ViewState.DataLoaded(result)
}
analyticsLogger.logEvent(
eventName = ANALYTICS_EVENT_LOAD_DATA,
parameters = mapOf(
ANALYTICS_PARAM_USER_ID to userRepository.getUserId(),
ANALYTICS_PARAM_SCREEN_NAME to "some_screen",
)
)
}
}
} catch (e: Exception) {
_viewState.value = ViewState.Error
analyticsLogger.logEvent(
eventName = ANALYTICS_EVENT_LOAD_DATA_ERROR,
parameters = mapOf(
ANALYTICS_PARAM_USER_ID to userRepository.getUserId(),
ANALYTICS_PARAM_SCREEN_NAME to "some_screen",
)
)
}
}
}So nice and clean now, isn’t it?
Facade is, to my experience, very simple, yet useful and overlooked pattern. If you can reduce complexity of crucial parts of your code, you should really consider using it.
2. Not cleaning up dead code in the project
Have your app stopped using the old registration form fragment? Maybe you re-wrote an old screen from XML to Compose? Or maybe there are some UI components lurking around the project that are not used anymore?
Remove unused code, please.
You won’t need it. And it’s not about keeping the project clean and tidy. Not removing dead code may waste some of your fellow devs time.
You might ask “but how?”. There are few possibilities.
But before I’ll list them, we have to clarify: it’s not always obvious if code is actually dead. Even IDE may lie to you (or don’t tell the whole truth).
- Even if it’s impossible to navigate to certain screen in your app, it might not appear as unused in your IDE. There might be a reference to its fragment in navigation graph, there might be a reference to its view model in a Koin/Dagger/Hilt module.
- It might also appear as unused, even though it’s used. For example, if you implement CastOptionsProvider and only reference it in manifest, it will appear as unused.
That being said, because of the ambiguity, your fellow developers might do some unnecessary work in code that should already be removed, or because of code that should already be removed.
I. Redundant refactoring
Your team might choose to migrate out of some library, introduce some data model or retract from a sketchy solution. If you keep dead code, you will force yourself or your fellow developers to perform refactoring on dead code, and ultimately waste time.
II. Obfuscated navigation
It’s essential to navigate through your codebase, either with Command + F or finding method usages. If you keep dead code, it will navigate yourself or your fellow developers to, making development, debugging and maintenance harder for no reason, ultimately resulting in a waste of time.
Thanks for reading my friend! I write more about Android, Kotlin and programming itself, so if you’re interested in this topic, consider following and seeing my other articles.
And also, let’s connect!
https://www.linkedin.com/in/apoplawski96/
