General revamp of Dagger usage #5

mdelolmo · 2017-01-15T13:14:19Z

Use of Builders+Dagger 2.7 way to define sub-components
Abstraction layer between injections and Dagger framework

- Use of Builders+Dagger 2.7 way to define sub-components - Abstraction layer between injections and Dagger framework

fSergio101 · 2017-01-15T19:00:03Z

Cool @mdelolmo!! I'm going to start a review of this PR giving you as much feedback as possible just to wonder myself if I'm understanding the way you implemented it. Feel free to correct my misunderstandings.

fSergio101

Ok!! I think that I got the main idea, that is to delegate the configuration of the dependencies, and the creation of the components linked to Activities, Fragments, etc to an external collaborator that could be plugged as kind of strategy pattern. We could see that object like a big injector, or a big class related to all app injection... meybe could be splitted... The reference to that Object implementation is Static and our App instance allow us to access it.

But I still miss one important detail: how do you swap the behaviours in testing time? do you retain the instance of the injector AKA ComponentProxy in App instance and create setters and getters for allowing you to swap the behavior? how do you do that magic?

If That would be like this, I have a new question: then all the testing config for activities and fragment would be depending on that object, I mean everything would be initialized in that moment because we have no way to get in the middle of an activity or fragment initialization, isn't it?

fSergio101 · 2017-01-15T19:13:32Z

app/src/main/java/me/martinez/sergio/daggertwobasearchitecture/activities/MainActivity.java

+
+    @Override
+    protected MainActivityComponent initDI() {
+        MainActivityComponent activityComponent = ((App) getApplication()).getComponentProxy().getMainActivityComponent(this);


Here I realized that you're delegating MainActivityComponent creation out of the activity. I guess you've implemented this method injection in order to be able of taking control of the creator by using the right implementation of yourComponentProxy interface. I like it! 👍

fSergio101 · 2017-01-15T19:19:30Z

app/src/main/java/me/martinez/sergio/daggertwobasearchitecture/activities/MainActivity.java

+    }

+    private void initSection() {
+        sectionComponent = ((App)getApplication()).getComponentProxy().getSectionComponent(this);


Same here, delegate into a new piece the creation of component, you're like this hiding the dependencies you have, it is a great idea.

fSergio101 · 2017-01-15T19:24:13Z

...main/java/me/martinez/sergio/daggertwobasearchitecture/activities/MainActivityComponent.java

-  A provideA();
-  List<String> provideDiInjectionHistory();
-  Log4Me provideLog4Me();
+public interface MainActivityComponent {


This interface is the one you use in your proxy facade and clients like MainActivity. And is the one that your implementation strategies "Test" and "prod" MUST implement in order to be swapped at "ComponentProxy" implementation point.

fSergio101 · 2017-01-15T19:27:09Z

.../java/me/martinez/sergio/daggertwobasearchitecture/activities/MainActivityComponentProd.java

+
+@PerActivity
+@Subcomponent(modules = {ActivityModule.class, MainActivityModule.class})
+public interface MainActivityComponentProd extends MainActivityComponent {


Here, is there point where you really have your dependencies and you can provide another MainActivityComponent implementation like MainActivityComponentProd using your mock dependencies. That's part of the magic. isn't it?

Yes, abstraction layers everywhere, I think it's improvable, in the way that you may not need to expose the subcomponent builder in this interface, which would make the interface kind of useless whatsoever.

fSergio101 · 2017-01-15T19:30:55Z

.../java/me/martinez/sergio/daggertwobasearchitecture/activities/MainActivityComponentProd.java

+import me.martinez.sergio.daggertwobasearchitecture.fragments.MainFragmentComponent;
+
+@PerActivity
+@Subcomponent(modules = {ActivityModule.class, MainActivityModule.class})


Looking at this @Subcomponent annotation I've realized that you have changed Appcomponent-MainActivityComponent relationship from a Component relationship into a subcomponent one. I wonder If that is strictly necessary. As far as I know it could keep as Component relationship. Did you commit this change because of any reason I'm misunderstanding maybe?

You mean that before the relationship was depends and now is a subcomponent? I decided to migrate the whole thing to the new API, perhaps I misunderstood that part and not everything needs to be a subcomponent.
For what I know, having it as sub-component is not the only way to make it get the corresponding bindings, but it would make sense if you want to define some sort of hierarchy. I think for Application-> Activity would make sense.

fSergio101 · 2017-01-15T19:49:17Z

app/src/main/java/me/martinez/sergio/daggertwobasearchitecture/di/ComponentProxyProd.java

+import me.martinez.sergio.daggertwobasearchitecture.fragments.sections.testsection.secondstep.SecondFragmentComponent;
+import me.martinez.sergio.daggertwobasearchitecture.fragments.sections.testsection.secondstep.SecondFragmentModule;
+
+public class ComponentProxyProd implements ComponentProxy {


This is the implementation you have in production code, here you really work with all the implementation details and component configuration of the injection points.

fSergio101 · 2017-01-15T20:15:51Z

app/src/main/java/me/martinez/sergio/daggertwobasearchitecture/di/ComponentProxyProd.java

+import me.martinez.sergio.daggertwobasearchitecture.fragments.sections.testsection.secondstep.SecondFragmentComponent;
+import me.martinez.sergio.daggertwobasearchitecture.fragments.sections.testsection.secondstep.SecondFragmentModule;
+
+public class ComponentProxyProd implements ComponentProxy {


There is one small detail I don't like too much. This class looks like is going to increase its size in an exponential way. I'm struggling about wich could be the best way to split it into different classes and make a composition.

At this point maybe would be interesting to use different parameterized interfaces in order to give more semantic to all the methods and having a contract with more consistency. That even would give you one extra point for Interface segregation principle If you take ComponentProxy interface and you split it into FragmentInjector<COMPONENT, DEPENDENCY, TARGET>, ActivityInjector<COMPONENT, DEPENDENCY, TARGET> like this you could from your fragment for instance:

fragmentComponent = ((App) getActivity().getApplication()).getFragmentInjector().getComponent(getActivity(), this);

I think that if you declare an interface for the method getComponent parametrized you don't need to cast it and if you want yo make it activity use

FragmentInjector<COMPONENT, DEPENDENCY extends Activity, TARGET extends Fragment>{ COMPONENT void getComponent(DEPENDENCY activity, TARGET fragment) }

I think that should work and looks better, let me know about your opinion and tell me if I am missing something, please.

I completely agree. This is the part I like the least too. Hopefully, I can use the fork of the project as a playground to find a work around.
That idea of yours sounds worth a try, by all means.

fSergio101 · 2017-01-15T20:29:03Z

app/src/main/java/me/martinez/sergio/daggertwobasearchitecture/di/ComponentProxyProd.java

+    }
+
+    @Override
+    public MainActivityComponent getMainActivityComponent(MainActivity mainActivity) {


And here is where the other part of the magic takes place, isn't it?
If I would like to mock some activity dependencies, maybe should I...

Create a new implementation of ComponentProxy for instance an extension of ComponentProxyProd that overrides this method should be fine, isn't it?

Create an implementation of MainActivityComponent with my new depency config, AKA my modules provisioning the mocked objects.

Create an implementation of AppComponent able to plug my implementation of `MainActivityComponent.

Return in the overridden method my new MainActivityComponent

That is, you may even mock ComponentProxy to return only the component you like to be tested, but you got it all right.

fSergio101 · 2017-01-15T20:45:57Z

build.gradle

  }
  dependencies {
-    classpath 'com.android.tools.build:gradle:1.2.3'
+    classpath 'com.android.tools.build:gradle:2.2.3'


Out of scope, but just FYI: I think that the change of this android gradle version (and wrapper) allows you to delete the apt plugin from gradle dependencies since annotationProcessor already has been included in new android gradle versions.

Did not know. Thanks for the tip ;)

fSergio101 · 2017-01-15T20:46:54Z

app/build.gradle

-  compile 'com.google.dagger:dagger:2.0'
+  compile 'com.android.support:appcompat-v7:25.1.0'
+  compile 'com.google.dagger:dagger:2.7'
+  apt 'com.google.dagger:dagger-compiler:2.7'


Read below and Try to change it by
annotationProcessor 'com.google.dagger:dagger-compiler:2.7'

fSergio101

Please move the PR destination to testing-pipeline I've created just now since I want to merge it but I think that master is still not the best branch to do it because I want to make some changes by myself. GREAT JOB DUDE!!! 👏 👏 👏 👏 👍

General revamp of Dagger usage

f81a19a

- Use of Builders+Dagger 2.7 way to define sub-components - Abstraction layer between injections and Dagger framework

fSergio101 reviewed Jan 15, 2017

View reviewed changes

fSergio101 requested changes Jan 15, 2017

View reviewed changes

General revamp of Dagger usage #5

Are you sure you want to change the base?

General revamp of Dagger usage #5

Uh oh!

Conversation

mdelolmo commented Jan 15, 2017

Uh oh!

fSergio101 commented Jan 15, 2017

Uh oh!

fSergio101 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fSergio101 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants