Refs #147 - Dagger Improvements

[spipau]

Where do you have this from? Can you share with me the source? I'd like to also read a bit about it first to better understand your changes, thanks!

[rajasone]

@spipau please checkout this link

[spipau]

@rajasone Thanks for the link, interesting read. I already used the DaggerFragment/Activities/Applications in the past, but then switched them for the Dex. But it works quite well with your implementation and we save some code. One question I still have, did you also think about removing all the @Singleton annotations as well? In the article you provided, they mention, that we should only use Singeltons when the object has a state. Which is mostly not the case in our code. I found this link, and the guy states:

"Just to be 100% clear things like okhttpclient, retrofit and gson should be declared @Reusable. right??"

Yes, in general I think it'd be good to declare stateless utilities and libraries as @Reusable. However, if they secretly keep some state—like handling connection limits or batching across all consumers—you might want to make them @Singleton, and if they are used very infrequently from a long-lived component it might still make sense to make them scopeless so they can be garbage-collected. It's really hard to make a general statement here that works for all cases and libraries: You'll have to decide based on library functionality, memory weight, instantiation cost, and expected lifespan of the objects involved.

[rajasone]

@spipau thanks for the review, Actually i was thinking to discuss with you about Scopes specifically Singleton we are using Singleton scope aggressively in our project and i think we can easily switch to @reusable or no scope for few instances e.g HttpLoggingInterceptor, Cache etc. Yes i am completely agree with you we should avoid Singletons, In our case i think having Dao instances Singleton make sense other than that we don't really need our instances of type Singleton.

[spipau]

@rajasone I agree and I'm very much open for discussion. But I honestly don't get scopes yet so much, I also didn't read much about it. Do you have experience with it? I also think we should reduce @Singelton as much as possible

[rajasone]

@spipau I have bit of the experience with Scopes let me know when you have time we'll schedule a call and share our understanding regarding scopes, or do you have any better idea? yes absolutely agree we should avoid Singleton as much as we can (Singleton and Custom scopes have double check in generated code (which is consider as performance overhead) @reusable have one check in generated code(one of the reason why it is preferable over Singleton) , may be we limit our scopes to only specific places e.g subcomponents for the sake of performance or if scopes are really necessary for other places e.g provider methods maybe we can use @reusable, that's my understanding and again maybe i am completely wrong so we have to double check it :)

[spipau]

I'll have to face the same things in SportsID and will there first try out these things and then implement them here. So this might take some time. But then I will know for sure more about these Dagger improvements. If you have further ideas, then go a head with the Scopes and implement them 😉

[rajasone]

@spipau okay i will play with it, hopefully we will find a good solution :)

[rajasone]

@spipau hi, thanks for the review :) Yes you are right may be splitting code for few provider methods are not that necessary, Reason why I split factory method is

• They were used in multiple provider methods and as far as I understand the @Reuseable annotation it means if this instance is found in cache return it otherwise create new one, so multiple provider methods use the same instance of types Gson, RxJava2CallAdapterFactory etc rather than creating new one eachtime.
• I think we saved few lines of code and Dagger is taking the responsibility of injection so why not just divide our responsibilities :laughing:

both of these changes are based on more towards personal preference, if you think these changes are not necessary we will remove these changes :smile: I don't have any other changes in my mind regarding this branch, so from my side it's good to go

[spipau]

I'm OK with keeping it split up, fine with me. And I have the same understanding of the @Reuseable annotation. But I was too fast with my last comment. We need to go back to more @Singelton implementations. I recently went through this project from Google and, as I also thought before, we should keep API and database classes as a Singelton. Why? Because Dagger generate Thread safe Singelton methods and they are essential here.

[rajasone]

@spipau cool, I reviewed the Dagger part of mentioned project, I am completely agree with you I'll make Api and Databases Singleton again