oldergod / android-architecture

MVI architecture Implementation of the ToDo app.
Apache License 2.0
669 stars 70 forks source link

Some feedback of the implementation #43

Closed littleGnAl closed 6 years ago

littleGnAl commented 6 years ago

Hello, @oldergod thank you for the amazing MVI implementation. I have been using it for a while in my project. I like this implementation very much, but I have encountered some problems, although they have been solved, I have been confused to do so, is it right or not.

  1. I am trying to write an accounting app for your implementation. I need to display a list of all accountings on the home page. I need to add a section header before the accounting of the day. The section header needs to be displayed the sum of the day's accountings because the list needs to be paged after paging, I don't know that the last accounting adapter item displayed is the last data of the day, so the sum can only be obtained from the database through SQL:

    private fun createHeader(createTime: Date): MainAccountingDetailHeader {
    val title: String = createHeaderTitle(createTime)
    // SELECT SUM(amount) FROM accounting
    val sum = getSumFromDatabase(oneDayFormat.format(createTime))
    val sumString: String = applicationContext.getString(
        R.string.main_accounting_detail_header_sum, sum
    )
    return MainAccountingDetailHeader(title, sumString)
    }

    You can check out my implementation here https://github.com/littleGnAl/Accounting/blob/591a8705bffebc79690f8bb716b35f81dc8d183c/app/src/main/java/com/littlegnal/accounting/ui/main/MainActionProcessorHolder.kt#L77. My solution is to put the observeOn() method behind scan() and let the reducer switch to the main thread after it has finished executing.

  2. There is a page that requires the user to select some and fill in some information, such as username, email, address, etc. The definition of ViewState is as follows:

    data class UserInfoViewState(
    val isLoading: Boolean,
    val error: Throwable? = null,
    val firstName: String? = null, 
    val lastName: String? = null, 
    val email: String? = null, 
    val phone: String? = null, 
    val address: String? = null
    )

    When the user clicks the confirmation button, it checks whether the information is valid. It will be saved the to the server if it is valid. I tried to save this information in ViewState, but I can only check if the information of preViewState is valid in the reducer. But in the reducer cannot call the Retrofit API to save the data, my current solution is to put these user information into the UserInfoValueHolder, and then in the corresponding processorto check whether the value of UserInfoValueHolder is valid, if valid directly call The API saves the user information to the server:

    
    data class UserInfoValueHolder(
    val firstName: String? = null, 
    val lastName: String? = null, 
    val email: String? = null, 
    val phone: String? = null, 
    val address: String? = null
    )

class UserInfoActionProcessorHolder { private var userInfoValueHolder: UserInfoValueHolder? = null

...

}


This solves the problem, but `userInfoValueHolder` is not immutable and can be changed anywhere, which is not what I want to see. I don't know if you can understand the problem I have expressed and whether you have encountered similar problems. I want to listen to your suggestions and look forward to your reply. Thank you.
oldergod commented 6 years ago

Hi

  1. I think that all the info required inside your createHeader, createHeaderTitle could be fetched inside the logic, and be included into the logic's Result, so the reducer wouldn't have to hit the network or database, but only compute data.

  2. I know that having access to the state from inside the logics is something important that I didn't address yet. I'm not sure I'll take the time to do that soon. You can check libraries like RxRedux that seems to allow that easily.

littleGnAl commented 6 years ago

@oldergod Thank you for your reply. For 1, but if I need to create adapter items based on the previous state, this solution will not work, such if the previous adapter items are empty, I should also add a header for it, you can check this: https://github.com/littleGnAl/Accounting/blob/591a8705bffebc79690f8bb716b35f81dc8d183c/app/src/main/java/com/littlegnal/accounting/ui/main/MainActionProcessorHolder.kt#L332

oldergod commented 6 years ago

Everything could be done inside a logic I think. You can still compose your stream after you've got a success object : multiple operators are no problem.

littleGnAl commented 6 years ago

Ok, thank you, I will think more about it.