Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I have this common design in my application:

public class MyListener1{
  MyTask1 task1;
  MyTask2 task2;

  public class MyListener1{

    public MyListener1(){
      task1 = new MyTask1(this);
      task2 = new MyTask2(this);
      task1.execute();
      task2.execute();
    }

    public void onResult1(String result){
      //extract result
    }

    public void onResult2(String result){
      //extract result
    }
}

public class MyTask1 extends AsyncTask{

  MyListener1 myListener1;

  public MyTask1(MyListener1 myListener1){
    this.myListener1 = myListener1;
  }

  @Override
  public void onResult(String result){
    myListener1.onResult1(result);
  }

}

public class MyTask2 extends AsyncTask{

  MyListener1 myListener1;

  public MyTask2(MyListener1 myListener1){
    this.myListener1 = myListener1;
  }

  @Override
  public void onResult(String result){
    myListener1.onResult2(result);
  }

}

The AsyncTask is a library class that execute the code in a method doInBackground() in another thread when calling execute(), and call onResult(String result) when task finished.

Of course, I summarize the code, MyTask1 and MyTask2 have a little bit more code, but I soon realized that they could be factorize on a common task:

public class MyListener1{
  MyTask task1;
  MyTask task2;

  public class MyListener1{

    public MyListener1(){
      task1 = new MyTask(0,this);
      task2 = new MyTask(1,this);
      task1.execute();
      task2.execute();
    }

    public void onResult(int taskId, String result){
       switch(taskId){
         case(0) : //extract result from task 0
         case(1) : //extract result from task 1
    }
}

public class MyTask extends AsyncTask{

  int id;
  MyListener1 myListener1;

  public MyTask(int id, MyListener1 myListener1){
    this.id = id;
    this.myListener1 = myListener1;
  }

  @Override
  public void onResult(String result){
    myListener1.onResult(id, result);
  }

}

I heard some people telling me to avoid "switch case" to identify a source and prefer to use "polyphormism" instead. In that case, I don't know what they mean.

So here is my question:

Is there a better way to identify the source (the object calling) of a callback method than using an identifier for the caller?

share|improve this question
1  
Something is not quite clear: you say that MyTask1 and MyTask2 have a little bit more code, but then you just merge them in one class. Is the "little bit more code" the same for both tasks? –  toto2 Apr 20 at 14:05
    
Something does not feel quite right about the listener referencing the tasks and the listener referencing the tasks. If you could please explain the whole project in more details. –  toto2 Apr 20 at 14:08
    
Also, do you really need to share the listeners between both tasks? or could you have a different listener for each task? –  toto2 Apr 20 at 14:12
    
@toto2 Yes, I can extract the code that is not the same between the two task and manage it somewhere else, so that I can merge MyTask1 and MyTask2 into MyTask About explaining it, Listener1 is actually in my project a ctrl for an Activity (it's an Android project). This ctrl does some HTTP request to a JSON API. The request are manage by the tasks, and call back the ctrl when they get the response. Finally, some ctrl need to do several tasks (remote call) so one "listener" can be the same for several task (that why I choose this example). –  Pierre-Jean Apr 20 at 15:01

4 Answers 4

up vote 6 down vote accepted

A listener and callback mechanism is a common pattern in many places in Java.

The logical place to look for examples is in the Swing API. Will get back to that in a second, but, there are two items that are useful first:

  1. Instead of using an int value to track the source of the event/Task, you should use the task itself.

       public void onResult(String result){
         myListener1.onResult(this, result);
       }
    
  2. The Listener itself should implement an interface, something like: TaskResultListener:

     public interface TaskResultListener {
         public onResult(Task task, String result);
     }
    

    and the constructor for the Task should take an instance of a TaskResultListener instead of a MyListener1

For examples of how this is used, consider the ActionListener interface and usage in Swing:

In the Swing API, it is common to have support for multiple listeners. You have just one. That is OK.

share|improve this answer
    
I agree for the interface, but why comparing reference is "proper" than comparing id? In other word, why if (task==myTask1){//do something} else if (task==myTask2){//do something else} is better than if (id == myTask1.ID) {//do something} else if (id == myTask2.ID) {//do something else} ? –  Pierre-Jean Apr 20 at 18:45
1  
@Pierre-Jean - because you could have duplicate ID's, you only use the ID's to differentiate between tasks, and there is an established precedent/best practice to do it another way (using the class instance). –  rolfl Apr 20 at 18:47

You could directly use the source object. This is commonly used in Swing when the same ActionListener has been added to more than one button. When the listener's actionPerformed method is invoked, the EventObject object that is passed featrues a getSource method that retrieves the source of the event. Then, you compare that source with button objects you have previously saved as fields in your class, and eventually perform a different action based on the equality comparison result.

share|improve this answer
    
Is there a reason why reference comparison id better than id comparison? –  Pierre-Jean Apr 20 at 18:46
    
@Pierre-Jean reference comparison is just another form of identifier comparison, except the identifier is managed by the virtual machine for you. –  Giulio Piancastelli Apr 21 at 15:34

Despite your explanation in the comments above, it is still not quite clear what you are really doing.

I believe I understand that Listener1 will be used by more than one Activity. However, it would be helpful to know what Listener1 needs to access in each Activity, or if it needs to access anything at all.

An option would be to not have a Listener1 at all. We could just define the AsyncTask's in the Activity and let them do their job. For example, create JsonFetchAgeAsyncTask and JsonFetchGenderAsyncTask and just override their onResult callbacks so that each one can modify something in the Activity.

I somewhat understand that you do this in different Activity's and you want to re-use your AsyncTask's subclasses. You could instead define your AsyncTask's within an abstract parent Activity class and then make children Activity's. Those children Activity's would share the same AsyncTask's.

Another option would be to use composition, which is actually what you are doing by sharing Listener1 between different Activity's. But it seems a bit off: I would at least change the name (not Listener) and I would define Task1 and Task2 within that class.

I would need more details to give you better advice. More specifically, I'm not sure how Listener1 is related to the containing Activity's.

share|improve this answer

I prefer your original code. It avoids the need for the switch by using two different classes, which (assuming they and their callback functions are well documented and have good names) is usually clearer and easier to understand.

If you have duplicated code between your MyTask1 and MyTask2 classes, have you considered introducing a new superclass for them?

Or, alternatively, if you're using (or could upgrade to) Java 8, how about passing in a lambda function argument to the task object, if the callback is the only difference between them?

share|improve this answer
1  
Android forked a long time ago from Java. I think they are still stuck at something which is close to Java 6 and there is no plan to bring Android closer to Java. –  toto2 Apr 20 at 17:10
    
Didn't realise Android was involved... yes, I should have guessed based on the class names, but it wasn't actually mentioned anywhere in the question. (Although what you say isn't entirely true -- a recent update added support for most of the new features of Java 7 -- I do suspect it will be a while before Android gets Java 8 features supported) –  Jules Apr 21 at 4:01
    
Thanks. I was not aware of it, but Android 4.4 does support Java 7. I had looked into this a long time ago and I was left with the impression that they would not even try to keep Android close to Java. –  toto2 Apr 21 at 22:58

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.