In this article we’ll go through how to own a legacy code that is 3 years old, how we can make it up and running again, and at the end, we’ll do some code reviews on the legacy code!

• Whether for professional work, a personal app, or a hobby, maintaining an exciting and robust Android codebase is key.
• Three years ago, I embarked on a project named FlickrSearch, aimed at leveraging Flickr’s Search API.

The Beginning

From Concept to Launch

• My journey started with the article “Launching a New Android Project in 2021,” covering everything from Kotlin DSL to Compose, MVVM, modularization, and Hilt.
• I’m here to discuss the successes and failures of my design choices over three years and how they can guide your development process.

Revisiting My 2021 Tech Stack

In 2021, I utilized:

• Android Databinding
• Kotlin DSL
• MVVM architecture
• Android ViewModels
• Dagger Hilt
• Kotlin Coroutines
• Android LiveData
• OkHttp, Retrofit, Moshi
• However, I omitted implementing tests.

These technologies were quite new and standard back in 2021. Luckily, we’ve got newer technologies to use in 2024. Most of the list needs to go, or at least could gradually be refactored. It’s important to note that if you’re working on a large codebase or have other priorities, a gradual refactor plan would be ideal.

One thing I strongly regret is not writing tests in 2021 and not doing TDD. As time passes, I understand more the importance of having tests in the codebase, especially when we want to do some refactoring. This is a common pattern I’ve seen in many codebases over the past 13 years of my working experience on 30+ Android projects. Either the codebase has some tests and developers maintain some of it, adding a few minimal unit tests from time to time, or the project has no tests/broken tests that are commented out, and no one dares to fix or add new tests.

The problem with this approach is that the app becomes a sinking ship in the long term. At a point, it becomes impossible to maintain new features as the code becomes really fragile, and every change in some part of the codebase breaks some other parts easily. It also becomes really hard to leverage new technologies and frameworks like Compose, etc.

A good recipe for ending up in this situation is:

• Not writing any type of tests 🚫
• Not maintaining legacy tests 🛠️
• Not updating dependencies in the project 🔄
• Not caring about the project build time ⏳
• Not caring about the app’s runtime performance 🏃‍♂️
• Not planning regular refactorings 📅

Then, we can easily end up in a deadlock situation where we either need to throw the codebase away and reinvent the wheel the right way, or spend countless hours on the technical debt we’ve accumulated.

Uncle Bob has many famous quotes about testing, emphasizing the importance of a “Test First” approach.

You cannot refactor without a comprehensive suite of tests that you trust with your life. If you have that suite, refactoring is always easy and free of risk. A moment of refactoring becomes a trivial thing rather than a huge risk.

— Uncle Bob Martin (@unclebobmartin) June 2, 2019

More on testing first:

https://blog.cleancoder.com/uncle-bob/2013/09/23/Test-first.html?source=post_page—–1a0323cfb457——————————–

Let’s get started and get our hands on the code!

first, let’s check if we can open the project in Android Studio, You can follow along by trying to open the project from this commit which is the last commit from 3 years ago!

1-Run the project

After 3 years the project does not seem to run on the recent version of Android Studio, Jellyfish Canary 11

Let’s update some dependencies to get it up and running again. check the following commit for the dependencies updated which lets us run the app again!

https://github.com/LloydBlv/FlickrSearch/commit/4414be860cc0577ac7bd98da23df716b4a1f3675?source=post_page—–1a0323cfb457——————————–

This is what the app looks like now:

https://miro.medium.com/v2/resize:fit:828/format:webp/1*rKnEtRNO7GD5uSxYaj4LYQ.gif

As you can see the app is quite simple, a search bar that gets the user’s query and looks for that search query on Flickr.

2- Updating dependencies

After running the project, we’d like to find out how can we update our dependencies, one way would be to manually look for every dependency which is time-consuming and boring, what about some plugins that we can use to find out what versions are available at the moment:

There are many tools available for this purpose, we can use the following for now:

https://github.com/ben-manes/gradle-versions-plugin?source=post_page—–1a0323cfb457——————————–

xWe add this library to the project then we get a nice report about currently available dependency updates as follows:

 

Now we can go ahead and update our dependencies at the project manually, we don’t have to look for the recent versions ourselves.

You can find the commit including these changes here:

 

https://github.com/LloydBlv/FlickrSearch/commit/d7431549a5e88aee21cf1bcff6213f355c77a6d1?source=post_page—–1a0323cfb457——————————–

 

So far we have gotten the project up and running after 3 years and updated all the dependencies to the recent version.

Now it’s time for some code review:

I want to take time and review design decisions that have been made before like it’s a PR/MR code review. This might be useful for understanding future code refactors.

The code review will be around how can we improve the existing tech stack and won’t include let’s use technology X instead.

@HiltViewModel
class SearchViewModel @Inject constructor(
        private val searchUseCase: SearchUseCase,
        @IoDispatcher val ioDispatcher: CoroutineDispatcher
) : ViewModel() 

CoroutineDispatcher should not directly be injected into the ViewModel, as the Android documents mention here the suspended functions should be safe to call from the main thread, meaning where that this suspended call (executing SearchUseCase) is being called should move to a suitable thread, This pattern makes your app more scalable, as classes calling suspend functions don’t have to worry about what Dispatcher to use for what type of work. This responsibility lies in the class that does the work.

Instead this should be moved to the repository layer, where it’s first provided to the classes using it.

This is how the repository layer is looking like now:

class SearchRepositoryImpl @Inject constructor(
        private val searchApi: SearchApi,
        private val searchDataMapper: SearchDataMapper) : SearchRepository {
    override suspend fun search(tag: String): List<FlickrPhoto> {
        return searchDataMapper.map(searchApi.search(tag))
    }
}

After moving the CoroutineDispatcher it will look like this:

class SearchRepositoryImpl @Inject constructor(
        private val searchApi: SearchApi,
        private val searchDataMapper: SearchDataMapper,
        @IoDispatcher val ioDispatcher: CoroutineDispatcher
) : SearchRepository {
    override suspend fun search(tag: String): List<FlickrPhoto> = withContext(ioDispatcher) {
        searchDataMapper.map(searchApi.search(tag))
    }
}

and our ViewModel looks cleaner now:

@HiltViewModel
class SearchViewModel @Inject constructor(
        private val searchUseCase: SearchUseCase,
) : ViewModel() {

Another thing that could be improved from the code above is the injection of SearchUseCase, It can be defined as dagger.Lazy so it won’t be injected until it is needed and we will have a better runtime and performance when using this ViewModel:

@HiltViewModel
class SearchViewModel @Inject constructor(
        private val searchUseCase: dagger.Lazy<SearchUseCase>,
) : ViewModel()

Another thing that I am not really happy with is the way that the query is passed from UI to the use case, It’s a combination of LiveData and Kotlin Flows, and I find it hard to read and understand, This is how it’s looking like at the moment:

@HiltViewModel
class SearchViewModel @Inject constructor(
        private val searchUseCase: dagger.Lazy<SearchUseCase>,
) : ViewModel() {

    private val searchQuery = MutableStateFlow("")

    private val _uiState = MutableLiveData<SearchUiState>()
    val uiState = _uiState

    init {
        viewModelScope.launch {
            searchQuery.debounce(DEBOUNCE_TIME_IN_MILLIS)
                    .collectLatest { query ->
                        Timber.d("collectLatest(), query:[%s]", query)
                        if (query.isEmpty()) {
                            _uiState.value = SearchUiState.Idle
                            return@collectLatest
                        }
                        try {
                            _uiState.value = SearchUiState.Loading
                            val photos = searchUseCase.get().invoke(query)
                            if (photos.isEmpty()) {
                                _uiState.value = SearchUiState.EmptyResult
                            } else {
                                _uiState.value = SearchUiState.Success(photos)
                            }
                        } catch (e: Exception) {
                            _uiState.value = SearchUiState.Error(e)
                        }
                    }
        }
    }

    fun onQueryChanged(query: String?) {
        query ?: return
        searchQuery.value = query
    }
}

A good practice is to avoid having any code inside init {} blocks specially in the ViewModels,

The code is refactored to the following:

@HiltViewModel
class SearchViewModel @Inject constructor(
    private val searchUseCase: dagger.Lazy<SearchUseCase>,
) : ViewModel() {

    private val searchQuery = MutableStateFlow("")

    val uiState: LiveData<SearchUiState> = searchQuery
        .debounce(DEBOUNCE_TIME_IN_MILLIS)
        .asLiveData()
        .switchMap(::createUiState)


    private fun createUiState(query: @JvmSuppressWildcards String) = liveData {
        Timber.d("collectLatest(), query:[%s]", query)
        if (query.isEmpty()) {
            emit(SearchUiState.Idle)
            return@liveData
        }
        try {
            emit(SearchUiState.Loading)
            val photos = searchUseCase.get().invoke(query)
            if (photos.isEmpty()) {
                emit(SearchUiState.EmptyResult)
            } else {
                emit(SearchUiState.Success(photos))
            }
        } catch (e: Exception) {
            emit(SearchUiState.Error(e))
        }
    }

    fun onQueryChanged(query: String?) {
        query ?: return
        searchQuery.value = query
    }

There’s no more logic in the constructor of the ViewModel, the code is more readable and requires less cognitive load to understand the logic.

A low-hanging fruit would be the following code block:

sealed class SearchUiState {
    object Loading : SearchUiState()
    object Idle : SearchUiState()
    data class Success(val photos: List<FlickrPhoto>) : SearchUiState()
    object EmptyResult : SearchUiState()
    data class Error(val exception: Throwable) : SearchUiState()
}

the objects could be converted to a data object instead:

sealed class SearchUiState {
    data object Loading : SearchUiState()
    data object Idle : SearchUiState()
    data class Success(val photos: List<FlickrPhoto>) : SearchUiState()
    data object EmptyResult : SearchUiState()
    data class Error(val exception: Throwable) : SearchUiState()
}

In the SearchAdapter we have the following code:

class SearchAdapter : ListAdapter<FlickrPhoto, SearchAdapter.ViewHolder>(DIFF_UTIL) {

    companion object {
        private val DIFF_UTIL = object : DiffUtil.ItemCallback<FlickrPhoto>() {
            override fun areItemsTheSame(oldItem: FlickrPhoto, newItem: FlickrPhoto): Boolean {
                return oldItem.id == newItem.id
            }

            override fun areContentsTheSame(oldItem: FlickrPhoto, newItem: FlickrPhoto): Boolean {
                return oldItem.title == newItem.title &&
                        oldItem.url == newItem.url
            }
        }
    }
}

There’s no point in defining the DiffUtil.ItemCallback in here, we can easily move it to it’s own class instead:

object SearchItemDiffUtil : DiffUtil.ItemCallback<FlickrPhoto>() {
    override fun areItemsTheSame(oldItem: FlickrPhoto, newItem: FlickrPhoto): Boolean {
        return oldItem.id == newItem.id
    }

    override fun areContentsTheSame(oldItem: FlickrPhoto, newItem: FlickrPhoto): Boolean {
        return oldItem.title == newItem.title &&
                oldItem.url == newItem.url
    }
}  

Summary:

We’ve learned what are the recent tech trends in 2024 and how can we keep our dependencies updated and we did a little bit of refactoring. In the next article, we’ll continue with our refactoring until we can check all the goals for 2024.

If you’d like to follow along with step-by-step refactoring until the end, let me know by liking this article! ✨

This arctile is previously published on proandroiddev.com