How to correctly remove Object from List of Objects

Question

I'm trying to make List of Effects. Where class Effect has duration after which this Effect should be removed from List<Effect>. This is my class Effect:

public class Effect extends Thread {
    private String title;
    private Long duration;

    @Override
    public void run() {
        try {
            sleep(duration * 1000);
            //todo remove this Effect from Effects object

        } catch (InterruptedException e) {
            e.printStackTrace();
        }
    }
}

and another class with List<Effect> called Effects:

public class Effects extends Thread {
    private List<Effect> effects = new ArrayList<>();

    public void addEffect(Effect effect) {
        effects.add(effect);
        System.out.println(effects);
        effect.start();
    }

    public void removeEffect(Effect effect) {
        effects.remove(effect);
        System.out.println(effects);
        effect.stop();
    }
}

I am trying to do it this way(again class Effect):

public class Effect extends Thread {
    private String title;
    private Long duration;
    private Effects effects;

    public Effect(String title, Long duration, Effects effects) {
        this.title = title;
        this.duration = duration;
        this.effects = effects;
    }

    @Override
    public void run() {
        try {
            sleep(duration * 1000);
            //todo remove this Effect from Effects object
            effects.removeEffect(this);
        } catch (InterruptedException e) {
            e.printStackTrace();
        }
    }
}

Main method:

public class Main {
    public static void main(String[] args) {
        Effects effects = new Effects();
        Random random = new Random();
        String msg;
        do {
            System.out.println("Press any key to add effect");
            msg = new Scanner(System.in).nextLine();
            if (!msg.equals("q")) {
                effects.addEffect(new Effect("a", (long) random.nextInt(20), effects));
            }

        } while (!msg.equals("q"));
    }
}

But to me it doesn't look like this is the right choice because both objects are nested inside each other. Is this normal or is there another way to do this?

Answer 1

I think using ObjectFactory would be appropriate in this case to reduce coupling between Effect and Effects.

import java.util.*;

public class EffectDriver {

    public static void main(String[] args) throws InterruptedException {
        ObjectFactory factory = ObjectFactory.getFactory();
        Effects effects = factory.getInstance(Effects.class);

        Effect e1 = new Effect("e1", 1L);
        Effect e2 = new Effect("e2", 1L);

        effects.addEffect(e1);
        effects.addEffect(e2);

        e1.start();
        e2.start();
        e1.join();
        e2.join();
        System.out.println("Effects size = " + effects.getSize());
    }

}

final class ObjectFactory {

    private static ObjectFactory instance = new ObjectFactory();

    Map<Class, Object> classes = new HashMap<>();

    private ObjectFactory() {
    }

    public static ObjectFactory getFactory() {
        return instance;
    }

    public <T> T getInstance(Class<T> clazz) {
        classes.computeIfAbsent(clazz, aClass -> {
            try {
                return (T) aClass.newInstance();
            } catch (InstantiationException | IllegalAccessException e) {
                e.printStackTrace();
                throw new RuntimeException("Sorry...");
            }
        });
        return (T) classes.get(clazz);
    }

    }

    class Effect extends Thread {
        private String title;
        private Long duration;
        private ObjectFactory objectFactory = ObjectFactory.getFactory();

        public Effect(String title, Long duration) {
            this.title = title;
            this.duration = duration;
        }

        @Override
        public void run() {
            try {
                sleep(duration * 1000);
                objectFactory.getInstance(Effects.class).removeEffect(this);
            } catch (InterruptedException e) {
                e.printStackTrace();
            }
        }

        @Override
        public String toString() {
            return "Effect{" +
                    "title='" + title + '\'' +
                    ", duration=" + duration +
                    '}';
        }
    }

    class Effects {
        private List<Effect> effects = new ArrayList<>();

        public void addEffect(Effect effect) {
            effects.add(effect);
            System.out.println(effects);
        }

        public void removeEffect(Effect effect) {
            System.out.println("Removing " + effect);
            effects.remove(effect);
            System.out.println(effects);
        }

        public int getSize() {
            return this.effects.size();
        }
    }

Answer 2

The use case that you mentioned seems to be something similar to cache. Guava has cache with expiry already implemented. You can read about it here: https://github.com/google/guava/wiki/CachesExplained

That being said, there are other issues with the problem you are trying to solve here:

1. You are creating multiple threads (each for an item in the list) which will sleep for the duration and remove the element once sleep is over. With multiple threads come concurrency issues as ArrayList is not thread safe.

2. Even if you use a thread safe collection, the number of threads would be of the magnitude of the elements in the array which by design is very inefficient.

3. When the thread sleep is over, it simply means the thread goes from (timed) waiting state to runnable state. It is not guaranteed to run immediately. So the expiry is not exact.

Now if this not exact behaviour is acceptable I would suggest refactoring your code to move out the responsibility of expiring element from Effect class.

1. Effects class would also contain an absolute expiry timestamp derived from duration of Effect class. This could be saved as another data structure in Effects class like a map of timestamps to set of effects.

2. In addEffect and removeEffect methods of Effects class, both data structures need to be modified.

3. A separate thread will run on regular intervals removing all the effect objects that are to be expired using the second data structure.

Answer 3

Object references are passed by value, as are primitive types in Java and the changes in another section will not effetc. See this Is Java “pass-by-reference” or “pass-by-value”?

If it possible for you, use static class to implement your algorithm. Something such as below:

public class Effects {
  private static List<Effect> effects = new ArrayList<>();

  public static void addEffect(Effect effect) {
    effects.add(effect);
    System.out.println(effects);
    effect.start();
  }

  public static void removeEffect(Effect effect) {
    effects.remove(effect);
    System.out.println(effects);
    effect.stop();
  }
} 

public class Effect extends Thread {
  private String title;
  private Long duration;

  public Effect(String title, Long duration) {
     this.title = title;
     this.duration = duration;
  }

  @Override
  public void run() {
    try {
        sleep(duration * 1000);
        //todo remove this Effect from Effects object
        Effects.removeEffect(this);
    } catch (InterruptedException e) {
        e.printStackTrace();
    }
  }
}

Answer 4

But to me it doesn't look like this is the right choice because both objects are nested inside each other. Is this normal or is there another way to do this?

You are right, you have more coupling than necessary. Typically each time another class need to be aware that the effect has ended, if would a referenced to another class to warn it.

Observer design pattern

What you can do is to use the Observer design pattern: https://en.wikipedia.org/wiki/Observer_pattern

Basically in Java you woudl define a interface EffectObserver with a method effectStopped:

public interface EffectObserver {
  void effectStopped(Effect e);
}

Then your effect would allow anyone interrested to register itself to get notifications and also define a way to notify observers of the event:

public class Effect {
  [...]
  List<EffectObserver> observers = new ArrayList<>();
  
  void addObserver(EffectObserver observer) {
    observers.add(observer);
  }
  
  void notifyStopped() {
    for(EffectObserver o : observers) {
      o.effectStopped(this);
    }
  }
  [...]
}

In your run method you would add the call to notifyStopped:

@Override
public void run() {
    try {
        sleep(duration * 1000);
    } catch (InterruptedException e) {
        e.printStackTrace();
    } finally {
      // Notify even if an exception occured.
      notifyStopped();
    }
}

Then the Effects class can just register itself to the effects instance it create and remove them from the list. Beware that because you are in multi thread case, you would need to synchronize things:

public class Effects extends Thread implements EffectObserver {
    private List<Effect> effects = new ArrayList<>();

    public void addEffect(Effect effect) {
        effects.add(effect);
        effect.addObserver(this);
        System.out.println(effects);
        effect.start();
    }    

    @Override
    synchronized void effectStopped(Effect e) {
        effects.remove(effect);
        System.out.println(effects);
    }
}

To go beyond that

• You extend Thread class to modify it, this is considered bad design in particular as starting/stopping a thread is long and heavy on ressources. Please prefer using a thread pool and submitting tasks to the pool.

• There no reason for the class Effects to extend Thread at all.

• In the example above, you do nothing with the list of Effects, why bother putting them in a list and store them and go the length and removing them latter on from that list. If the list of effects is not really used, don't model it at all. It would be much simpler.

• Don't block a Thread with thread.sleep this waste system resources. You can schedule tasks to be executed at a given time in the future thanks to ScheduledExecutorService class. If your CPU usage is low (as this is the case in current implementation), an executor with a single thread would be likely be able to deal with thousand of concurrent effects.