ViktorArtemiev / Surveys

Nimble Technical Test
0 stars 0 forks source link

A few DI opinions #4

Closed sleepylee closed 5 years ago

sleepylee commented 5 years ago
ViktorArtemiev commented 5 years ago

Hi @sleepylee Trung

The Dagger implementation seems to be out-of-dated.

Do not quite understand what's the problem with this. I am trying to follow the official documentation with some modification. Would be great if you could give more specific details regarding the current DI implementation.

At your MainViewModel, surveyRepository supposed to be used only to create dataSourceFactory, why wouldn't we just inject dataSourceFactory and let add a graph to our DI scoped for this VM?

Good point, agree. Improved.

DataModule seems to hold too many responsibilities, this could cause to a later expand. Also, AuthorizationInterceptor could be extracted no? (btw, for oauth, I'd recommend to try OkHttp's Authenticator instead of intercepting manually by Interceptor, just my 2 cents)

Agree Implemented refreshing Token by using OkHttp Authenticator and extracted in a separate module. Thanks!

pull-request linked #12

nhphong commented 5 years ago

Hello @ViktorArtemiev, my name is Phong. I'm an Android engineer at Nimble. To answer your question regarding the recent DI implementation:

This is just one way of implementing dagger-android. There're many other different ways, but the key thing is that we make use of DispatchingAndroidInjector to map our dependencies to their proper "component" (injector).

ViktorArtemiev commented 5 years ago

Hello @nhphong Phong, Nice to meet you 👋and thank you very much for the explanation. DI implementation updated accordingly.

PR linked #18