badoo / MVIKotlin

Extendable MVI framework for Kotlin Multiplatform with powerful debugging tools (logging and time travel), inspired by Badoo MVICore library
https://arkivanov.github.io/MVIKotlin
Apache License 2.0
827 stars 64 forks source link

make Reducer a fun interface? #191

Closed tieskedh closed 3 years ago

tieskedh commented 3 years ago

Maybe it's an idea to make Reducer a fun-interface?

arkivanov commented 3 years ago

This sounds like a good idea! Would you like to submit a PR?

dalewking commented 3 years ago

You want a PR for just adding the keyword "fun" in front of the interface?

arkivanov commented 3 years ago

Well yes. I can do it by myself probably later this week.

dalewking commented 3 years ago

While you're at it ViewRenderer should also be a fun interface

arkivanov commented 3 years ago

I would keep ViewRenderer as it is unless there is a use case.

dalewking commented 3 years ago

What is the use case for NOT making ViewRenderer a fun interface. Basically any interface with a single abstract non-suspend method should probably be a fun interface unless there is a compelling reason not to

The use case is that I am not using MviView mostly because I don't want to use ViewEvents on Android since I am using FlowBinding library to create a Flow directly without all the boiler plate. So I deal with a ViewRender directly, but even if using BaseMviView it has a field where you can set a ViewRenderer.

If I don't use diffing I have to do something like this to create a ViewRenderer:

         val renderer = object: ViewRenderer<State> {
            override fun render(model: State) {
                label.text = model.value
            }
        }

Adding fun to ViewRenderer lets you simplify to

         val renderer = ViewRenderer<State> { label.text = it.value }

Your reaction implies to me that you may not fully understand the benefits of a fun interface. There is a great article on the subject.

arkivanov commented 3 years ago

I think in your case you can just use normal functions, so your renderer becomes just a consumer of ViewModels: (Model) -> Unit. Regarding BaseMviView: you can just override its render method, no need to assign the renderer property. The ViewRenderer interface is not meant to be created anonymously, but if you think it makes sense then let's make it fun interface as well.

dalewking commented 3 years ago

I think in your case you can just use normal functions, so your renderer becomes just a consumer of ViewModels: (Model) -> Unit.

Except that is not type compatible with diff function

Regarding BaseMviView: you can just override its render method, no need to assign the renderer property.

But you allow for it so someone could want to use the property

The ViewRenderer interface is not meant to be created anonymously, but if you think it makes sense then let's make it fun interface as well.

Which was my point because I do want to create it anonymously

arkivanov commented 3 years ago

Could you kindly provide a code snippet? From my point of view, you either use diff or normal Kotlin functions. In neither case you need to create ViewRenderer anonymously.

dalewking commented 3 years ago

So in my case I have no MviView implementation class. The controller onViewCreated looks like this:

fun onViewCreated(
    viewLifecycle: Lifecycle,
    renderer: ViewRenderer<State>,
    intents: Flow<Intent>,
    eventHandler: suspend (Event) -> Unit
) {
    bind(viewLifecycle, BinderLifecycleMode.CREATE_DESTROY) {
        intents bindTo store
        store.states bindTo renderer
        store.labels bindTo eventHandler
    }
}

In my Android fragment I have:

        val intents = merge(
            button1.clicks().map { SomeIntent },
            button2.clicks().map { AnotherIntent },
            ...
        )

        val renderer = object: ViewRenderer<State> {
            override fun render(model: State) {
                label.text = model.data
            }
        }

And then to call the controller I have:

        controller.onViewCreated(
            viewLifecycleOwner.lifecycle.asMviLifecycle(), renderer, intents
        ) {
            // handle labels here
        }

So I could switch the controller parameter to just take (Model) -> Unit and change the renderer field to just be a lambda BUT if I decided later that the renderer did need to use diff I would also have to switch the controller to ViewRenderer. So it is best to stay with ViewRenderer so the Controller doesn't know the difference.

Making ViewRenderer a functional interface means I can use this to declare the renderer:

        val renderer = ViewRenderer<State> { label.text = it.data }

and if I want to switch renderer to diff I can say:

        val renderer = diff { diff(State::data) { label.text = it } }

without changing the Controller at all

arkivanov commented 3 years ago

Thanks, that is a valid use case indeed. Will love to make ViewRenderer a fun interface.

dalewking commented 3 years ago

That sure was a lot of discussion to get a total of 4 characters added to a file

arkivanov commented 3 years ago

This is because I want all changes to be reasonable.

arkivanov commented 3 years ago

Looks like marking an interface as fun may be a breaking change.

For example, the following case fails with StackOverflowError after making ViewReneder a fun interface:

    override fun <Model : Any> Observable<Model>.bindTo(viewRenderer: ViewRenderer<Model>) {
        this bindTo {
            assertOnMainThread()
            viewRenderer.render(it)
        }
    }

This function becomes recursive once ViewRenderer is marked as fun, because now this lambda is treated as ViewRenderer due to SAM conversion.

I think I will keep ViewRenderer as it is for now. You can add the following function to reduce the boilerplate:

inline fun <T : Any> viewRenderer(crossinline render: (T) -> Unit): ViewRenderer<T> =
    object : ViewRenderer<T> {
        override fun render(model: T) {
            render.invoke(model)
        }
    }