bufferapp / android-clean-architecture-boilerplate

An android boilerplate project using clean architecture
MIT License
3.66k stars 519 forks source link

What purpose in setPresenter() method inside View wich has injection of that's presenter #27

Open mong0ose opened 6 years ago

mong0ose commented 6 years ago

https://github.com/bufferapp/android-clean-architecture-boilerplate/blob/db8130da5a40ecac2a43fb55bd6e65c466266e7f/mobile-ui/src/main/java/org/buffer/android/boilerplate/ui/browse/BrowseActivity.kt#L21

Hello, I figured that you set presenter in BrowseActivity using dagger injection and method setPresenter() which I think would be redundant becouse of using dagger. Maybe you can correct me, if I'm mistaken.

Also I'm pretty excited about this article with new vision of clean architecture, and your second topic with new architecture components from google is good too. Nice work!

katien commented 6 years ago

I was wondering about this too. I can remove fun setPresenter(presenter: T) from the BaseView and rely on dependency injection to give the view a presenter reference. I'd like to know the rationale for having setPresenter() instead of nothing or just a val presenter: T which would be initialized by dagger.

Another thing which seemed redundant is the @Inject annotation on the presenter's constructor here:

class BrowseBufferoosPresenter @Inject constructor(
    val browseView: BrowseBufferoosContract.View, 
    val getBufferoosUseCase: SingleUseCase<List<Bufferoo>, Void>, 
    val bufferooMapper: BufferooMapper): BrowseBufferoosContract.Presenter {

We already provide the BrowseBufferoosPresenter in our BrowseActivityModule, so removing this annotation doesn't seem to affect anything. I was thinking that these might just seem redundant in the example app and have some purpose in a larger scale application though. Any insight would be appreciated.

qwertyfinger commented 6 years ago

setPresenter function is indeed redundant. There are different ways to associate Presenter and View, so maybe it stayed there by mistake. BaseView is nice to have for a clear hierarchy, even if it doesn't contain anything. And maybe it will contain some other base functions depending on your requirements.

qwertyfinger commented 6 years ago

@devKate Regarding @Inject annotation – in the current setting it is also redundant. But what I like to do is to use it and instead remove the corresponding @Provides function. There are several reasons for that:

}

- If we were to add a new constructor parameter to the Presenter, we would have to duplicate the change in `@Provides` function each time.
- With constructor injection we can switch to `@Binds` functions which should be preferred to `@Provides` functions.

Now in order to do that in Presenter we need to use concrete Use Case implementation, not the `SingleUseCase` interface.
It's hard for me to see the case in which we would need to substitute one Use Case interface with different implementations.
And using `val getBufferoosUseCase: GetBufferoos` instead of `val getBufferoosUseCase: SingleUseCase<List<Bufferoo>, Void>` seems much less verbose and more clear to me.
But, of course, if for some reason you need to use interfaces, then you have to stick with `@Provides` functions.
If not, then let's summarise:
- You get shorter and more comprehensible (IMO) constructor parameter declarations for Presenter.
- You get more concise, more readable and perhaps even slightly more performant `@Module` class implementation:

@Module abstract class BrowseActivityModule {

@Binds
abstract fun provideBrowseView(
    browseActivity: BrowseActivity
): BrowseBufferoosContract.View

@Binds
abstract fun provideBrowsePresenter(
    browseBufferoosPresenter: BrowseBufferoosPresenter
) : BrowseBufferoosContract.Presenter

}