Hey guys, I am a junior developer and at work we have a screen where the user input basic details about himself: Firstname, Lastname, phone,etc. The UI updates the ViewModel when the “continue” button in the UI is tapped. I wonder if we “over-engineer” our ViewModel or is it just me having it hard to grasp. This is the ViewModel:

class BasicDetailsViewModel (private val basicDetailsRepository: BasicDetailsRepository) : ViewModel() {

    val mUserBasicDetailsPlaceHolderData = basicDetailsRepository.getBasicUserDetailsPlaceholderViaIdentityProvider()
    private val mUserBasicDetailsRequestData: MutableLiveData<UserBasicDetailsRequest> = MutableLiveData()

    /**
     * Upon the user taps the continue button, we update [mUserBasicDetailsRequestData]
     * with the data the user has input.
     */
    fun onContinueClicked(firstName: String, lastName: String, phone: String, profileImageUrl: String) {
        mUserBasicDetailsRequestData.value = UserBasicDetailsRequest(firstName, lastName, phone, profileImageUrl)
    }

    var updateUserRequest: LiveData<NetworkCall<User>> = mUserBasicDetailsRequestData.switchMap { data ->
        liveData(<http://Dispatchers.IO|Dispatchers.IO>) {
            emit(NetworkCall<User>(NetworkStatus.LOADING, null, ""))
            val updatedUser = basicDetailsRepository.updateUserBasicDetails(data)
            emit(updatedUser)
        }
    }

After the user input his basic data and taps the “continue” button in the UI, the following happens: • The value “mUserBasicDetailsRequestData” in the ViewModel is changed with the data sent from the UI. • The value “updateUserRequest” in the ViewModel is observing mUserBasicDetailsRequestData, and gets triggered using “switchMap” which I find somewhat confusing. • “updateUserRequest”, upon triggered, updates our backend with the basic user data, and notify the UI that the backend has successfully received our user basic details. In the UI, we then observe updateUserRequest:

mViewModel.updateUserRequest.observe(this, Observer<NetworkCall<UpdateExpertiseFieldsResponse>> { networkCall ->
    when (networkCall.status) {
        NetworkStatus.SUCCESS -> handleSuccess() //Hide loader & do something.
        NetworkStatus.ERROR -> handleError(networkCall.message ?: getString(R.string.authentication_general_error)) //Hide loader & show error to the user.
        NetworkStatus.LOADING -> showLoader() //Show loader.
    }
})

I wonder, is there a special reason that we need this additional variable “updateUserRequest”? Why do we need a “switchMap” here? In my mind I would just do something like this, when the “continue” button is clicked.

fun onContinueClicked(firstName: String, lastName: String, phone: String, profileImageUrl: String) {
        mUserBasicDetailsRequestData.value = UserBasicDetailsRequest(firstName, lastName, phone, profileImageUrl)

        liveData(<http://Dispatchers.IO|Dispatchers.IO>) {
            emit(NetworkCall<User>(NetworkStatus.LOADING, null, ""))
            val updatedUser = basicDetailsRepository.updateUserBasicDetails(data)
            emit(updatedUser)
        }
    }

This is of course just a simple example, and I asked it since we repeat this pattern a couple of times. Am I missing some important consideration I might not be aware of?

That's not too bad. For me, for UI code to be considered good, there must be 1) separation between the state and state transitions 2) the view should only be aware of the current state and 3) there is no state transition happening in the view but it must be able to trigger them from somewhere else. That's what's happening with the

LiveData

member of the view model. The view triggers the state transition by calling a view model method and then new state comes in through the live data. I do not like that the actual request is a live data though. That's the reason for the

switchMap

. It should just be a call to a suspending function and a couple of state emissions into the resulting live data. The only problem I have with this are the names and granularity. Never mind the hungarian notation, the name of the live data suggests that the state transitions are too fine-grained. If that's indeed the only state in the screen then it's fine. In my opinion, there should only be one live data per screen (I am not a fan of MVVM). All state must be consolidated into a single sum type, each variant describing what possible states a screen can be in at any moment. Note the difference between "a screen's state" and "what state a screen is in". The latter should drive the former. This requires a bit of planning to achieve.

@mon Thank you! my write was a long read.. I appreciate your time and help 🙂 Can you please elaborate: How would you write differently to avoid the

switchMap

at the request part? as for the hungarian notation part - can you please provide an example what part of it can be improved?

If I were to rewrite it, it would look something like this:

interface BasicDetailsRepository {
    fun getBasicUserDetailsPlaceholder(): User
    suspend fun updateBasicUserDetails(
        firstName: String, lastName: String, phone: String, profileImageUrl: String
    ): User
}

sealed class BasicDetailsScreen {
    object Busy : BasicDetailsScreen()
    class Loaded(val user: User) : BasicDetailsScreen()
    class Failed(val cause: Throwable) : BasicDetailsScreen()
}

class BasicDetailsViewModel(
    private val repository: BasicDetailsRepository
) : ViewModel() {
    private val _state = MutableLiveData<BasicDetailsScreen>().also {
        it.value = BasicDetailsScreen.Loaded(repository.getBasicUserDetailsPlaceholder())
    }

    val state: LiveData<BasicDetailsScreen>() = _state

    fun updateDetails(
        firstName: String, lastName: String, phone: String, profileImageUrl: String
    ) {
        _state.value = BasicDetailsScreen.Busy
        viewModelScope.launch {
            try {
                val updatedUser = repository
                .updateUserBasicDetails(firstName, lastName, phone, profileImageUrl)
                _state.value = BasicDetailsScreen.Loaded(updatedUser)
            }
            catch (error: Throwable) {
                _state.value = BasicDetailsScreen.Failed(error)
            }
        }
    }
}

then in the fragment:

val basicDetails: BasicDetailsViewModel by viewModels { vmFactory }
basicDetails.state.observe(viewLifecycleOwner) {
    when (it) {
        BasicDetailsScreen.Busy -> TODO("show a spinner or something")
        is BasicDetailsScreen.Loaded -> TODO("sync views with the data from `it.user`")
        is BasicDetailsScreen.Failed -> TODO("crash or notify the user and return to clean state")
    }
}
// also call `basicDetails.updateUser(...)` in a view listener somewhere

Thanks, got it! 😄 🍻 @mon One last question please You changed from use of

livedata(<http://Dispatcher.IO|Dispatcher.IO>)

to

viewModelScope{}

Is there something potentialy wrong with my use of

livedata(<http://Dispatcher.IO|Dispatcher.IO>)

?

There's nothing wrong, it's probably even needed depending on how the repository is implemented. That's just my preference because the

viewModelScope

can cancel pending requests when it's getting cleared