MVVM damage just by bad naming?

Table of Contents

The offender

Imagine a store product screen, it may contain a ViewModel that looks something like this. Any concerns?

class ViewModel {
    // public UI state
    val uiState: Observable<UiState> = ..

    // internal state
    private var count = 0

    // public functions
    fun fetchExtraDetails() { .. }
    fun persistCartInDb() { .. }
    fun tryIncrement() { if (canIncrement) count++ .. }
    fun navigateToForwardOrToPayment() { navigate(if (needsPayment) .. else ..) }
}

Technically it’s great:

• It exposes an observable state for the View.
• It knows nothing of the View.
• It exposes public functions for View to manipulate the model.
• It keeps the logic (canIncrement/needsPayment) in the VM instead of View - good separation and testability.

But I’m saying it’s an absolute disaster:

• VM implementation details are exposed
• To pick a correct function the View as to understand VM implementation.
• It would not look odd if some of these conditionals were implemented on View side instead.
• VM tests will look redundant, cryptic or both. Not revealing much about the View it is Modeling.

And all of this is due to the naming of VMs public methods. I’d consider it a major defect. Lint wouldn’t catch it. Luckily the fix can be very cheap.

The fix

class ViewModel {
    // public UI state
    val uiState: Observable<UiState> = ..

    // internal state
    private var count = 0

    // public functions
    fun onShowExtras() { .. }
    fun onSave() { .. }
    fun onIncrement() { if (canIncrement) count++ .. }
    fun onNavigateForward() { navigate(if (needsPayment) .. else ..) }
}

All I’ve done is i treated the public API of the ViewModel as an abstraction of the View it is Modeling. Meaning the naming should expose the capabilities of the View: “show extras”, “save” etc. It should not expose whatever the ViewModel needs to make it happen.

Consider them declarative: what is happening, instead of how it should be happening.

Note: the “on” prefix is not necessary. But it’s common in Android where it signals that something has happened, like onResume.

Note: onSaveClicked etc. would also make sense, but the concern is that it unnecessarily couples the exact interaction type - Click/Tap/Swipe, with the name.

Takeaways

View should be dumb

For testability, a View should be dumb. It can not be truly dumb if it has to orchestrate business logic. Calling persistCartInDb() implies View dictates it’s time to actually persist (what if you want to add validation also?). In case of invoking onSave(), the View just declares what happened in UI, with no concern on what should follow.

Correct naming protects the quality

Imperative naming invites for imperative code. Some junior can be excused to think they can wrap persistCartInDb in some if-else statement on the View side. Leading to misuse of MVVM and weakening of your presentation layer. Declarative naming, like onSave, should already hint that any handling belongs to the VM side.

ViewModel tests are View logic tests

Therefore, for a testcase name, would you rather read:
when persistCartInDb is invoked then cart is presisted in db
Or
when onSave is invoked then cart is presisted in db
I’d argue that the latter style reads like a specification of the feature, much more informative. One of the big reasons for using VM is to be able to test the View logic, so it should make sense it reads somewhat like a View test - not hyperfocused on the VM internals.

Extremes

Public fields

What if you avoid this whole naming issue by having a public count:

class ViewModel { ..
    public var count = 0
.. }

This is an extreme end of the same violation. You completely lose track of when and why count is modified. View (and VM) can call it any time from any place. There is no connection between capabilities of the view and the change of the model state. Testing it is meaningless. Instead of reading the code you’ll be jumping through the invocation points to make any sense.

MVI

class ViewModel {
    // public actions
    sealed interface Intent {
        data object ShowExtras : Intent
        data object Save : Intent
        data object Increment : Intent
        data object NavigateForward : Intent
    }

    // public UI state
    val uiState: Observable<UiState> = ..

    // internal state
    private var count = 0

    // public function
    fun onIntent(intent: Intent) {
        when (intent) { .. }
    }
}

This is the another extreme. In MVI you don’t call individual methods on VM, instead you have a single method that takes the action as a parameter. This very formalized approach eliminates the original issue all together. When using MVVM, thinking how it would be in MVI serves as a very good guideline.