I think one of the main reasons we end up writing code that doesn’t make any sense is because we don’t really understand what our code does. You get to a point where the code kind of works and you assume you are done. This is specially true when you deal with complex libraries like Rx or Dagger2 where the learning curve is really steep and you default to StackOverflow or tutorials to copy & paste some code.

In this specific case we are going to have a look at a code that someone wrote using Dagger2, the goal was quite simple, using Dagger we wanted to inject the same instance of MyClass. If you know about Dagger this shouldn’t be a really complicated task, you could just annotate the constructor with @Inject and @Singleton (or the appropriate scope) and that’s it.

@Singleton

public class MyClass { @Inject

public MyClass() { ... } }

or

@Module

public class MyModule { @Provides

@Singleton

public MyClass provideMyClass() { return MyClass(...);

} }

What happened?

Injecting a class via annotating its constructor or instantiating it in a module is just one of the steps. You also need a component and you have to be aware of scopes and what they do.

You will indeed have just one instance of the class as long as you don’t create a new component every single time. You have to reuse that component somehow across your app (usually by put it in your Application class). Because the person who was doing this didn’t understand Dagger he started to see different instances all the time so he said “I don’t trust Dagger”.

He didn’t know why this @Singleton annotation wasn’t creating a single instance and to make matters worse he didn’t understand scopes and never bothered to check the code generated by Dagger, if he had done that he would have seen that when you annotate with a scope, Dagger actually returns the same instance as long as you don’t create more instances of the same component. The Dagger generated code below shows that it uses a DoubleCheck (effective Java 71) to create that singleton instance.

@SuppressWarnings("unchecked") private void initialize(final Builder builder) { this.provideMyClass =

DoubleCheck.provider(

MyModuleModule_ProvideMyClass.create(builder.myModule)

); }

The code below is what he merged to master.

The solution was to create a singleton within a singleton plus bonus points for using AtomicReference (because of threading issues). Did this actually work? I have no idea but I would say no, I never checked it because as soon as we saw this code we revert it back and implemented it properly.

How did this code get into master?

This definitely highlighted a big internal issue, 90% of the team was at a conference so there was just one person that reviewed this code and thought it was OK. Because there wasn’t anyone else the code was merged.

To avoid this issue we put in place some rules about accepting merge requests when the majority of the team is not around. But the most important one was realizing that not everybody knows or has a great knowledge about complicated things like Dagger. People usually come to a project and everything is already set up so they never get to learn or understand how it works. To fix this gap we run a really long session about Dagger, explaining everything from the most simple concept to the strategy that we follow in our multi-module project and even going through the generated code line by line to debunk some myths. We also wrote some documentation about it and it’s part of our project wiki for everyone to read.