Ezike / Baking-App-Kotlin

Android architecture sample with dynamic feature modularisation, clean architecture with MVI (Uni-directional data flow), dagger hilt, DFM Navigation, kotlin coroutines with StateFlow and Exo player.
Apache License 2.0
464 stars 85 forks source link

data class *ViewState over sealed class for *ViewState #47

Closed mochadwi closed 4 years ago

mochadwi commented 4 years ago

Is there any concern why using data class RecipeViewState instead of using sealed class RecipeViewState?

Equivalent to your existing code using sealed class

sealed class RecipeViewState : ViewState {
 object Loading : RecipeViewState()
 object Refreshing : RecipeViewState()
 data class LoadedRecipes(val recipes: List<RecipeModel>) : RecipeViewState()
 data class LikedRecipe(val recipe: RecipeModel) : RecipeViewState() // adding a new state for liked item
 data class Error(val error: String?, val errorEvent: ViewEvent<String>?, val isEmpty: Boolean) // Unavailable, No data or empty etc
}

// in Fragment 
// instead of:
    override fun render(state: RecipeViewState) {
        when {
            state.isDataUnavailable -> binding.renderEmptyState(state)
            state.isDataAvailableError -> binding.renderDataAvailableErrorState(state)
            state.isNoDataError -> binding.renderNoDataErrorState(state)
            state.isLoading -> binding.renderLoadingState()
            state.isRefreshing -> binding.renderRefreshState()
            else -> binding.renderSuccessState(state)
        }
    }

// modifed to:

    override fun render(state: RecipeViewState) {
        when (state) {
            is Error -> {  
                if (state.error != null) binding.renderNoDataErrorState(state)
                if (state.errorEvent != null) binding.renderDataAvailableErrorState(state)
                if (state.isEmpty) binding.renderEmptyState(state) 
            }
            Loading -> binding.renderLoadingState()
            Refreshing -> binding.renderRefreshState()
            is LikedRecipe -> binding.renderLikedState(state)
            is LoadedRecipes -> binding.renderSuccessState(state)
            // no else branch
        }
    }

Pros:

  1. No need manually override every property/member of RecipeViewState using .copy with a new state
  2. No else branch in override fun render
  3. No need configuring a lot of function/method to determine state inside RecipeViewState, e.g for current approach: val *state: RecipeViewState get() = /* this.copy(isLoading = false, etc) */

@Ezike

Ezike commented 4 years ago

Is there any concern why using data class RecipeViewState instead of using sealed class RecipeViewState?

Equivalent to your existing code using sealed class

sealed class RecipeViewState : ViewState {
 object Loading : RecipeViewState()
 object Refreshing : RecipeViewState()
 data class LoadedRecipes(val recipes: List<RecipeModel>) : RecipeViewState()
 data class LikedRecipe(val recipe: RecipeModel) : RecipeViewState() // adding a new state for liked item
 data class Error(val error: String?, val errorEvent: ViewEvent<String>?, val isEmpty: Boolean) // Unavailable, No data or empty etc
}

// in Fragment 
// instead of:
    override fun render(state: RecipeViewState) {
        when {
            state.isDataUnavailable -> binding.renderEmptyState(state)
            state.isDataAvailableError -> binding.renderDataAvailableErrorState(state)
            state.isNoDataError -> binding.renderNoDataErrorState(state)
            state.isLoading -> binding.renderLoadingState()
            state.isRefreshing -> binding.renderRefreshState()
            else -> binding.renderSuccessState(state)
        }
    }

// modifed to:

    override fun render(state: RecipeViewState) {
        when (state) {
            is Error -> {  
                if (state.error != null) binding.renderNoDataErrorState(state)
                if (state.errorEvent != null) binding.renderDataAvailableErrorState(state)
                if (state.isEmpty) binding.renderEmptyState(state) 
            }
            Loading -> binding.renderLoadingState()
            Refreshing -> binding.renderRefreshState()
            is LikedRecipe -> binding.renderLikedState(state)
            is LoadedRecipes -> binding.renderSuccessState(state)
            // no else branch
        }
    }

Pros:

  1. No need resetting RecipeViewState using .copy for every `RecipeViewState)
  2. No else branch in override fun render

@Ezike

I'm not even sure why I used a data class anymore, but I remember I needed the list of recipes in a lot of the view states, so I thought I'd just use one class instead of many classes that take the recipe list as a parameter..

Also using copy isn't resetting the viewstate, it's making a new viewstate just like yours does. They're all good approaches.

mochadwi commented 4 years ago

Also using copy isn't resetting the viewstate, it's making a new viewstate just like yours does.

Thanks for clarifying! *I've corrected my description

I'm not even sure why I used a data class anymore, but I remember I needed the list of recipes in a lot of the view states, so I thought I'd just use one class instead of many classes that take the recipe list as a parameter..

I see, it's a matter of preference, then~

aliumujib commented 4 years ago

Nice conversation you've got going here, there are situations where you'd want to use copy, for example when you want to reuse data from a previous state. A concrete example of this is if you had a paged list of recipes and you wanted to append a new batch of recipes to the existing data in the list. So you'd essentially have to append the new data to the existing list, then make a new copy of the viewstate while replacing the old list with your new updated list like this:

var currentRecipeList = state.recipeList currentRecipeList.toMutableList().append(newRecipeList()) newState = state.copy(recipeList = newRecipeList)

That's a reason why I frequently use data classes instead of sealed classes to model State. Haven't yet found a cleaner approach. I'd gladly take suggestions however.

Sorry guys, I don't know how to make a new line appear within code snippets.

Ezike commented 4 years ago

Nice conversation you've got going here, there are situations where you'd want to use copy, for example when you want to reuse data from a previous state. A concrete example of this is if you had a paged list of recipes and you wanted to append a new batch of recipes to the existing data in the list. So you'd essentially have to append the new data to the existing list, then make a new copy of the viewstate while replacing the old list with your new updated list like this:

var currentRecipeList = state.recipeList currentRecipeList.toMutableList().append(newRecipeList()) newState = state.copy(recipeList = newRecipeList)

That's a reason why I frequently use data classes instead of sealed classes to model State. Haven't yet found a cleaner approach. I'd gladly take suggestions however.

Sorry guys, I don't know how to make a new line appear within code snippets.

Definitely, that makes sense. With sealed classes, you'd need to do a type check to achieve a similar behavior, since not all sub classes of the sealed class have the list as a property. For instance, you'd need to check if the previous state was a success state before gaining access to the old list.. Data classes are better suited for such cases.