Dependency Injection & refactored Dependency Management

[williamwebb]

Introduction of Dependency Injection through Dagger2. Required a lot of dependency management refactoring...

On the Dependency Management side it is far from optimized, look at MainViewCompanion & SettignsCompanion... Likely many of these dependencies where are being passed in directly could be encapsulated and structured better but would require more refactoring.

[martinbonnin]

I've never understood the hype behind dagger... What do we gain exactly doing this ?

[williamwebb]

You can replace dagger with any DI framework for the purposes of this PR. The main goal here is to create a clear logical graph of the dependencies through out the application(and have them be swappable via DI).

The one key thing you will notice from this PR is there are no longer any .get() static retrieval methods. Now there is a clear view of what depends on what, as well as a clear understanding and control of where the Context variable is being used.

[martinbonnin]

This PR adds 500 lines of code to a 5000 lines project...

[martinbonnin]

As for context, unless we are in the very short introduction activity, we always use the application context. There's a vendetta on static fields nowadays but I still don't get why...

[williamwebb]

This PR adds 500 lines of code to a 5000 lines project...

Correct, with the vast majority of that being the dagger code.

As for context, unless we are in the very short introduction activity, we always use the application context. There's a vendetta on static fields nowadays but I still don't get why...

There are many reasons why you should not use static fields. For me the two main reasons are,

1. They explicitly make unit testing impossible, you cant mock/stub/replace/inject a static field/method without writing your code to support it or doing some reflection magic.

2. Static fields represent global state, meaning the state of an object isn't dependent on itself or what is passed into it, instead it is magically dependent on some other object that could be in any state.

Over all the use of static variables and accessor methods is an anti-pattern as it allows you to do things that you shouldn't.

While this app is a little unique as it life cycle is directly dependent on its Application life cycle where most android apps are built around activities, we should still strive to have the best design possible.

[martinbonnin]

1. They explicitly make unit testing impossible, you cant mock/stub/replace/inject a static field/method without writing your code to support it or doing some reflection magic.

I actually wrote a small article about this some time ago: https://medium.com/@mbonnin/espresso-testing-without-dagger-2-3933fddeda74. This indeed requires to write code to support it and I believe this code is much more concise than the equivalent dagger 2 boilerplate.

It's true that if everyone understands dagger2, it might become the defacto standard at some point. Then dagger2 becomes interesting as a communication medium with other devs, but the technique of dependency injection and unit testing can be achieved through much more concise ways.

2. Static fields represent global state, meaning the state of an object isn't dependent on itself or what is passed into it, instead it is magically dependent on some other object that could be in any state.

Passing context as a parameter to the constructor does not give more information as to what state context is in. It does make the dependency more explicit at the price of more code. Note that imports also explicit the dependency.

Over all the use of static variables and accessor methods is an anti-pattern as it allows you to do things that you shouldn't.

I challenge the internet to show me what's so dangerous there :-D. BTW, I 'm pretty sure dagger uses static variables under the hood ;).

While this app is a little unique as it life cycle is directly dependent on its Application life cycle where most android apps are built around activities, we should still strive to have the best design possible.

All in all, and given that it's just you and me working on this app at the moment, I don't want to rush the merge. If more dev shows up and want all the dagger2 stuff then we'll go for it.

[williamwebb]

... This indeed requires to write code to support it and I believe this code is much more concise than the equivalent dagger 2 boilerplate.

This works but has two downsides, you are inherently leaking state by exposing these methods. Also your production code now is littered with code meant for debug/testing. Where with DI your production code knows no different.

...it might become the defacto standard at some point...

It already is :)

Passing context as a parameter to the constructor does not give more information as to what state context is in.

100% correct, in which case you should pass in the direct values, which DI facilitates.

I challenge the internet to show me what's so dangerous there :-D.

There are articles upon articles on the net going over this issue. Googling "android dev memory leak problem" and the first response is about static activities/views. But isn't the reduction of spaghetti code not enough reason?

BTW, I 'm pretty sure dagger uses static variables under the hood ;).

One of the beauties of Dagger is is all generated code, you can look at all the code produces and you will see it doesn't. Unless you mean internal to Dagger2, in which case I welcome you to find a non immutable static variable.

Like I said this MR is by no means tied to dagger2, if you for some reason want a different DI framework by all means we can go with that, but dagger2 is the best option for android. The point of this MR is DI, I hope this isn't what you are arguing against? Either way by no means do I want this rushed, there are a lot of changes here. It should be completely & thoroughly reviewed.

Message me on the Discord server when ever you get a chance and we can chat about it :).