Skip to content

Conversation

@AFGhazy
Copy link
Owner

@AFGhazy AFGhazy commented Jul 29, 2018

No description provided.

@AFGhazy AFGhazy changed the title No dagger2 nor data manager Add dagger2 support Jul 29, 2018
@Provides
@Singleton
Cache provideOkHttpCache(Application application) {
int cacheSize = 5 * 1024 * 1024;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AFGhazy extract this to a final constant


((BaseApp) getActivity().getApplication()).getNetworkComponent().inject(this);

setPresenter(NewTodoPresenter.getInstance(mTodosSubscriber, this));

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AFGhazy this should be handled by DI, add @Inject to the presenter do the necessary changes (add @ provide .. etc) and remove this line.

Copy link

@IslamSalah IslamSalah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AFGhazy +1 For the clean easy to review PR. Keep it up

@Module
public class NetworkModule {
@Provides
@Named("api_url")

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AFGhazy I believe @Named("api_url") is redundant and can be removed. What do you think?


@Provides
@Singleton
@Named("rxjava2_call_adapter")

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AFGhazy I believe @Named("rxjava2_call_adapter") is redundant and can be removed. What do you think?

@Provides
@Singleton
@Named("non_cached")
OkHttpClient provideOkHttpClientNonCached() {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AFGhazy I believe this method is never used. If so we can remove it and remove @Named("cached") as well. Kindly let me know if I missed something.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants