search

mgolokhov / dodroid

May the knowledge be with you!
MIT License
20 stars 12 forks source link

Introduce Dependency Injection #112

Closed mgolokhov closed 5 years ago

mgolokhov commented 5 years ago

Probably, based on Dagger 2

JubrilEdu commented 5 years ago

I'd start working on this @mgolokhov

mgolokhov commented 5 years ago

Cool!

JubrilEdu commented 5 years ago

So, I just checked the codebase and I think we have to move to MVVM first before this task can be achievable. Currently, it seems tightly coupled. What do you think? @mgolokhov

mgolokhov commented 5 years ago

@djubreel Retrofit, GoogleAnalytics, Crashlytics, Sound utility can utilize DI right away.

JubrilEdu commented 5 years ago

If that's the case can the issue be splitted? one for the dependencies you mentioned and maybe have the DI based on modules.

JubrilEdu commented 5 years ago

@mgolokhov Can you have a look at this? https://github.com/mgolokhov/dodroid/compare/master...djubreel:feature/DI?expand=1#diff-dbde4834a54fcb38f64a1d0091e36c24

it's a base implementation of DI in the project.

mgolokhov commented 5 years ago

@djubreel sorry for the late response, I was sick for more than a week. Thank you, it's a good start. Couple observations: Injecting from a base class Dagger 2 will only satisfy the base class's dependencies, not its subclasses. Current consumers for Tracker class are BaseApp and InterrogatorFragment The release build will crash.

I'm working on PR with MVVM, room, coroutines and dagger2 first version here https://github.com/mgolokhov/dodroid/pull/122

JubrilEdu commented 5 years ago

Hey @mgolokhov, I hope you are better now? I'd discontinue work on this and go through your branch that contains the changes.

mgolokhov commented 5 years ago

resolved in https://github.com/mgolokhov/dodroid/pull/122