Calling inject() from a Dagger Module method - bad practice?

Question

I'm still new to Dependency Injection and I try to implement my application using the MVP design pattern. I use a scoped graph for each view. I decided that my Adapter should be considered part of the View after reading through this great article.

The view in my example case is a Fragment. I created a module [referred to as FragmentModule] which provide its Presenter and View.

Before I started to mess around, the module only injected the Fragment and it provided the FragmentAdapter by calling its constructor with the required parameters.

Module:

@Module(
        overrides = true,
        includes = BaseFragmentModule.class,
        injects = {
                MyFragment.class,
        }
)
public class FragmentModule {

    private MyFragment mFragment;

    // ... // Other methods removed for clarity

    @Provides
    @Singleton
    public FragmentAdapter provideAdapter() {
        return new FragmentAdapter(mFragment.getActivity(), mFragment);
    }

    // ... //
}

The FragmentAdapter constructor looked somewhat like this:

private Context mContext;
private CustomListener mListener;

public FragmentAdapter(Context context, CustomListener listener)
{
    mContext = context;
    mListener = listener;
    // ... // 
}

This might still be "the correct way to do it" but I want to discuss the way I'm currently doing it, so keep on reading!

Then I rewrote it again, so that the FragmentAdapter took a reference to the Fragment instance. I then assigned Context and Listener within the FragmentAdapter constructor.

private MyFragment mFragment;

@Provides
@Singleton
public FragmentAdapter provideAdapter() {
    return new FragmentAdapter(mFragment);
}

The Adapter constructor looked like this:

private Context mContext;
private CustomListener mListener;

public FragmentAdapter(MyFragment iFragment)
{
    mContext = iFragment.getActivity();
    mListener = iFragment;
    // ... // 
}

After that I decided that I wanted to inject the Context to the adapter for some reason. So I continued: I made the FragmentModule inject also the Adapter, like so:

@Module(
        overrides = true,
        includes = BaseFragmentModule.class,
        injects = {
                FragmentAdapter.class,
                MyFragment.class,
        }
)

Now I had to learn how to inject the FragmentAdapter to the Fragment's scoped ObjectGraph in a nice, clean way. First I called inject() from the FragmentAdapter constructor:

FragmentModule:

// ... //
private MyFragment mFragment;
// ... //
@Provides
@Singleton
public FragmentAdapter provideAdapter() {
    return new FragmentAdapter(mFragment);
}
// ... //

FragmentAdapter:

@Inject Context mContext;
@Inject MyListener mListener;

public FragmentAdapter(MyFragment iFragment)
{
    iFragment.getObjectGraph().inject(this);
    // ... // 
}

Again, for some reason (remember I am learning..) - I wanted to get the injection to work without having to pass the Fragment instance to the FragmentAdapter constructor, so I ended up calling inject() from within the Module class:

    @Provides
    @Singleton
    public FragmentAdapter provideAdapter() {
        if (mAdapter == null) {
            mAdapter = new FragmentAdapter();
            mFragment.getObjectGraph().inject(mAdapter);
            mAdapter.initialize(); // Code moved from constructor which depends on injected members
        }
        return mAdapter;
    }

Now I am curious - what do you consider to be the best practice here? How would you implement the Adapter and Fragment dependencies with Dagger injection? And why?

Thanks a lot for your feedback!

Answer 1

The way I see it, ObjectGraph.inject(...) should be avoided as much as possible. Instead, you want to do Constructor Injection: pass your injected dependencies in your constructor.

You can do this by adding an @Inject annotation to you constructor:

private final Context mContext;
private final MyListener mListener;

@Inject
public FragmentAdapter(Context context, MyListener listener)
{
    mContext = context;
    mListener = listener;
    // ... // 
}

Now, you don't need the provideAdapter() anymore, since Dagger recognizes the @Inject annotation on the constructor. You do need to provide Context and MyListener instances, which I believe you are already doing.

That having said, I don't believe MyListener should be injected. It is not a dependency of your FragmentAdapter, but a feature. Just call setListener(MyListener) from your Fragment class.

A final remark, sometimes inject(...) cannot be avoided. Especially when you're using classes whose constructors you don't manage (such as Activity and Fragment).