current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top
up vote 4 down vote favorite
1

I have an Android application which needs to retrieve JSON files from a server and then display the information in my app. To accomplish this I have the following architecture:

app-architecture

  1. The main activity adds a job to the job queue requesting certain information from the server.
  2. The job gets run, retrieving the specified information and caching it in case the same request is made again in the future.
  3. The job on completion adds an event containing the information to the event bus.
  4. The event bus then relays the event back to the main activity which renders the information.

While I was developing other parts of the application I simply provided the job queue, event bus and cache via constants, i.e.

public class Dependencies {
  public static final JobQueue JOB_QUEUE = new JobQueue();
  public static final EventBus EVENT_BUS = new EventBus();
  public static final Cache CACHE = new Cache();
}

I knew at some point I wanted to use dependency injection (via the Dagger 2 library) to improve the style and make my code more modular and testable. I have now begun the process of migrating my code over to this style but I am discovering some problems. Here is my new code (some parts omitted):

public class MainActivity extends Activity {

  @Inject EventBus mEventBus;
  @Inject JobQueue mJobQueue;
  @Inject Cache mCache;

  private Session mSession;

  @Override
  protected void onCreate(final Bundle savedInstanceState) {
    super.onCreate(savedInstanceState);

    /*
    * Dagger 2 injection
    * */
    ((MyApplication) getApplication()).getDependenciesComponent().inject(this);

    final HttpUser httpUser = new HttpUser("[email protected]", "xxxxxxxx");
    startSession(httpUser);
  }

  @Override
  protected void onResume() {
    super.onResume();
    mEventBus.register(this);
  }

  @Override
  protected void onPause() {
    super.onPause();
    mEventBus.unregister(this);
  }

  @Subscribe
  public void onSessionStarted(final SessionStartedEvent sessionStartedEvent) {
    final boolean didExceptionOccur = sessionStartedEvent.didExceptionOccur();
    if(didExceptionOccur) {
      final Throwable throwable = sessionStartedEvent.getThrowable();
      handleSessionStartFailedThrowable(throwable);
    }else {
      mSession = sessionStartedEvent.get();
      loadTasks(mSession);
    }
  }

  @Subscribe
  public void onTasksLoaded(final TasksLoadedEvent tasksLoadedEvent) {
    final boolean didExceptionOccur = tasksLoadedEvent.didExceptionOccur();
    if(didExceptionOccur) {
      final Throwable throwable = tasksLoadedEvent.getThrowable();
      handleLoadTasksFailed(throwable);
    }else {
      final Task[] tasks = tasksLoadedEvent.get();
      final String taskArrayStr = Arrays.toString(tasks);
      Log.i(TAG, "tasks loaded: " + taskArrayStr);
      setTasksOnFragments(tasks);
    }
  }

  private void startSession(final HttpUser httpUser) {
    final Job startSessionJob = new StartSessionJob(this, mEventBus, httpUser);
    mJobQueue.addJobInBackground(startSessionJob);
    Log.d(TAG, "added start session job to job queue");
  }

  private void loadTasks(final Session Session) {
    final Job loadTasksJob = new LoadTasksJob(this, mEventBus, mCache, session);
    mJobQueue.addJobInBackground(loadTasksJob);
    Log.d(TAG, "added load tasks job to job queue");
  }

}

My primary focus here is on the job objects. Whereas before I would simply access the job queue, event bus, and cache from the static singletons, I now have to inject those dependencies into their constructors. As you can see for the "load tasks" job

final Job loadTasksJob = new LoadTasksJob(this, mEventBus, mCache, session);

I now have a four argument constructor, whereas before it was only two. I have read in "Clean Code" that a 4-arg constructor is an abomination and typically a sign you need to refactor your code. Is this the case here, am I doing something wrong structuring my code in this way?

share|improve this question
up vote 3 down vote accepted

The solution I developed was to inject the dependencies into the job queue instead of the jobs. This way the injection only has to happen once and can be accomplished nicely using Dagger 2 (note: because I was using the android-priority-jobqueue library I had to wrap it in my own class to allow for this functionality).

This is my Dagger module:

import android.app.Application;

import com.treemetrics.taskmanager3.util.AppCache;
import com.treemetrics.taskmanager3.work.eventbus.EventBus;
import com.treemetrics.taskmanager3.work.jobqueue.JobQueue;

import javax.inject.Singleton;

import dagger.Module;
import dagger.Provides;

@Module
public class AppModule {

  private final Application mApplication;

  public AppModule(final Application application) {
    mApplication = application;
  }

  @Provides
  @Singleton
  Application providesApplication() {
    return mApplication;
  }

  @Provides
  @Singleton
  EventBus providesEventBus() {
    return new EventBus();
  }

  @Provides
  @Singleton
  AppCache providesCache() {
    return new AppCache();
  }

  @Provides
  @Singleton
  JobQueue providesJobQueue(final Application app, final EventBus bus, final AppCache cache) {
    return new JobQueue(app, bus, cache);
  }

}

Dagger component:

import javax.inject.Singleton;

import dagger.Component;

@Singleton
@Component(modules = AppModule.class)
public interface AppComponent {

  void inject(MainActivity activity);

}

My wrapper class for the android-priority-jobqueue JobManager class which accepts the dependencies:

import android.content.Context;
import android.support.annotation.Nullable;

import com.birbit.android.jobqueue.JobManager;
import com.birbit.android.jobqueue.config.Configuration;
import com.treemetrics.taskmanager3.util.AppCache;
import com.treemetrics.taskmanager3.work.eventbus.EventBus;

public class JobQueue {

  private static final int DEF_PRIORITY = 1;

  private final Context mContext;
  private final JobManager mJobManager;
  private final EventBus mEventBus;
  private final AppCache mAppCache;

  public JobQueue(final Context context, final EventBus eventBus, final AppCache appCache) {
    mContext = context;
    final Configuration jobManagerConfig = new Configuration.Builder(context).build();
    mJobManager = new JobManager(jobManagerConfig);
    mEventBus = eventBus;
    mAppCache = appCache;
  }

  public void addJob(final Job job) {
    addJob(job, null);
  }

  public void addJob(final Job job, @Nullable final CharSequence groupId) {
    final String groupIdStr = (groupId != null) ? groupId.toString() : null;
    final BirbitJob birbitJob = getBirbitJob(job, DEF_PRIORITY, groupIdStr);
    mJobManager.addJobInBackground(birbitJob);
  }

  private BirbitJob getBirbitJob(final Job job, final int priority, @Nullable final String groupId) {
    return new BirbitJob(job, mContext, mEventBus, mAppCache, priority, groupId);
  }

}

I also have a new Job interface which my JobQueue wrapper class accepts instead of the Job class from the android-priority-jobqueue library. These jobs when submitted to my new JobQueue get translated to "birbit jobs" (the job class which the library accepts) and have the dependencies injected into them at that time. Here is my new Job interface and the BirbitJob class which is used to translate my jobs into the jobs accepted by android-priority-jobqueue.

(Notice the dependencies are provided in the onRun method.)

import android.content.Context;

import com.treemetrics.taskmanager3.util.AppCache;
import com.treemetrics.taskmanager3.work.eventbus.EventBus;

public interface Job {

  void onRun(Context context, EventBus bus, AppCache cache) throws Throwable;

  void onCancel(Throwable throwable, EventBus bus);

}

My translator job class BirbitJob:

import android.content.Context;
import android.support.annotation.NonNull;
import android.support.annotation.Nullable;

import com.birbit.android.jobqueue.Params;
import com.birbit.android.jobqueue.RetryConstraint;
import com.treemetrics.taskmanager3.util.AppCache;
import com.treemetrics.taskmanager3.work.eventbus.EventBus;

class BirbitJob extends com.birbit.android.jobqueue.Job {

  private final Job mJob;

  private final Context mContext;
  private final EventBus mEventBus;
  private final AppCache mAppCache;

  public BirbitJob(final Job job,
                   final Context context,
                   final EventBus eventBus,
                   final AppCache appCache,
                   final int priority) {
    this(job, context, eventBus, appCache, priority, null);
  }

  public BirbitJob(final Job job,
                   final Context context,
                   final EventBus eventBus,
                   final AppCache appCache,
                   final int priority,
                   final String groupId) {
    super(new Params(priority).requireNetwork().groupBy(groupId));
    mJob = job;
    mContext = context;
    mEventBus = eventBus;
    mAppCache = appCache;
  }

  @Override
  public void onAdded() {
  }

  @Override
  public void onRun() throws Throwable {
    mJob.onRun(mContext, mEventBus, mAppCache);
  }

  @Override
  protected void onCancel(final int cancelReason, @Nullable final Throwable throwable) {
    mJob.onCancel(throwable, mEventBus);
  }

  @Override
  protected RetryConstraint shouldReRunOnThrowable(
      @NonNull final Throwable throwable, final int runCount, final int maxRunCount) {
    return RetryConstraint.CANCEL;
  }

}
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.