Hey guys, I am a junior developer and at work we h...

# android-architecture

[Ofir Bar]

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?

[mon]

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.

[Ofir Bar]

@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?

[mon]

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

[mon]

hungarian notation is using names like mUserRepository for field members and sConstantValue for statics. Google does this for consistency with existing codebase, but even they wish they could drop those prefixes

[Ofir Bar]

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>) ?

[mon]

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