java.util.Observable but with generics to avoid casts

Question

This is an Observable class similar to java.util.Observable. The difference is that it uses generics to avoid casts. The question is: Is it really worth the effort? What are the problems with this code?

import java.util.ArrayList;
import java.util.Collection;

/**
 * like java.util.Observable, But uses generics to avoid need for a cast.
 *
 * For any un-documented variable, parameter or method, see java.util.Observable
 */
public class Observable<T>  {
   private boolean changed = false;

   // Holds registered observers.
   // @TODO if class is not final can we make this protected? (should we?).
   private final Collection<Observer<? super T>> observers;

   /**
    * observersSet gives the opportunity of changing the implementing set. The
    * benefit is, final user can specify the 'order' of observers notified.
    * Generally, not a good idea at all.
    *
    * @TODO should this be private and the class final?.
    */
   protected Observable(Collection<Observer<? super T>> observersSet) {
      this.observers = observersSet; }

   public static <F> Observable<F>getInstance() {
      // return getInstance(container);
      ArrayList<Observer<? super F>> container = new ArrayList<>();
      return new Observable<>(container);
   }

   /**
    * Before this method can be actually used, this.update(T arg) must be fixed.
    * It has a hard-coded ArrayList. Generally, not a good idea, not at all.
    *
    * User would expect the call getInstance(ArrayList<SomeObserver> ar) to
    * work however it wont. It must be:
    * getInstance(ArrayList<? extends SomeObserver> ar);
    *
    * It's bad, ugly, and more importantly, violating.
    *
    * Unless of course the first example could be made to work. Maybe a cast
    * will do.
    */
   @SuppressWarnings("UnusedParameters")
   public static <F> Observable<F>
      getInstance(Collection<Observer<? super F>> observersSet) {
      throw new UnsupportedOperationException("Not yet implemented.");
      // return new Observable<>(observersSet);
   }

   // @TODO null check maybe for add?
   public synchronized boolean add(final Observer<? super T> o) {
      return !this.observers.contains(o) && observers.add(o);

   }
   public void addObserver(final Observer<? super T> o)         { this.add(o); }

   public synchronized boolean rm(final Observer<? super T> o)  { return observers.remove(o); }
   public void deleteObserver(final Observer<? super T> o)      { this.rm(o); }

   public synchronized void clear() { this.observers.clear(); }
   public void deleteObservers()    { this.clear(); }
   // @TODO all of them need synchronized?
   public synchronized void setFlag() { this.changed = true; }
   public synchronized void clrFlag() { this.changed = false; }

   // Hei! did you know if you used projectLombok.org you could remove this
   // method? didn't you?!!! :O 0_o
   public synchronized boolean getFlag() { return this.changed; }
   public boolean hasChanged() { return this.getFlag(); }
   public int countObservers() { return this.observers.size(); }

   public void update(final T args) {
      ArrayList<Observer<? super T>> copy;

      synchronized (this) {
     if(!changed) return;
     copy = new ArrayList<>(this.observers);
     this.clrFlag();
      }
      // Look at OpenJDK to see why a new Collection is used.
      for(Observer<? super T> o : copy) o.update(args);
   }
   public void notifyObservers(final T args) { this.update(args); }

   public void update() { this.update(null); }
   public void notifyObservers() { this.update(); }
}

Observable now carries type of args sent to each Observer, and Observer is also aware of it, thus no cast needed.

---

The follow up question is located here.

Answer 1

A Generic Observable is a neat idea. Much of the code that was initially designed for the Observer pattern in Java was then actually built using a custom interface for each 'event', and you ended up, for example, with the Listener and Event interfaces that are so common in the AWT/Swing frameworks. A more general-purpose system has merits though.

What will be the usage pattern for it, though? Is there ever a need to create subclasses? Is there a need for the factory-methods you have created? Note, those factory methods cannot ever create instances of a sub-class.

Further, you have synchronization built in to it, but the synchronization is on the instance, which is unsafe (what if someone synchronized on your instance for some other reason, suddenly your locking strategy is confounded).

Finally, there's a reason that the Observer instance in the JDK has a two-argument notify, and that's because it needs to know where the notification came from. That way the Observer can observe multiple Observables, and know which one the notification originates from.

These are 4 significant issues I see in your design.

Your implementation is a bit shaky too:

• the indentation is a mess
• braced 1-liners
• poor variable names
• duplicate methods (deleteObserver calls rm, addObserver calls add, deleteObservers calls clear)
• unimplemented methods (getInstance())

All in all, there's a lot to consider, and this is not done the way I would expect.

If it were me, I would keep the generics a bit simpler, and would implement the code much closer to the original Observer, except adding the generics.

I have used your code as a template, but then restructured, and rewritten parts, to be:

import java.util.ArrayList;
import java.util.Collection;
import java.util.function.Supplier;

/**
 * like java.util.Observable, But uses generics to avoid need for a cast.
 *
 * For any un-documented variable, parameter or method, see java.util.Observable
 */
public class Observable<T> {

    public interface Observer<U> {
        public void update(Observable<? extends U> observer, U arg);
    }

    private boolean changed = false;
    private final Collection<Observer<? super T>> observers;

    public Observable() {
        this(ArrayList::new);
    }

    public Observable(Supplier<Collection<Observer<? super T>>> supplier) {
        observers = supplier.get();
    }

    public void addObserver(final Observer<? super T> observer) {
        synchronized (observers) {
            if (!observers.contains(observer)) {
                observers.add(observer);
            }
        }
    }

    public void removeObserver(final Observer<? super T> observer) {
        synchronized (observers) {
            observers.remove(observer);
        }
    }

    public void clearObservers() {
        synchronized (observers) {
            this.observers.clear();
        }
    }

    public void setChanged() {
        synchronized (observers) {
            this.changed = true;
        }
    }

    public void clearChanged() {
        synchronized (observers) {
            this.changed = false;
        }
    }

    public boolean hasChanged() {
        synchronized (observers) {
            return this.changed;
        }
    }

    public int countObservers() {
        synchronized (observers) {
            return observers.size();
        }
    }

    public void notifyObservers() {
        notifyObservers(null);
    }

    public void notifyObservers(final T value) {
        ArrayList<Observer<? super T>> toNotify = null;
        synchronized(observers) {
            if (!changed) {
                return;
            }
            toNotify = new ArrayList<>(observers);
            changed = false;
        }
        for (Observer<? super T> observer : toNotify) {
            observer.update(this, value);
        }
    }
}

Note that, apart from the generics, the signatures are the same as the existing Observable.

Additionally, the synchronization uses an internal instance, and I use Java8 suppliers to have a constructor giving a collection. That way you can get a safe instance for the synchronization.

The original Observer is not synchronized, and, this one does things a little differently in the notify method, because it does the notification after doing the changed=false change. The JavaDoc for the original Observable indicates that the change should only be reset after the notifications....

Answer 2

Pick a convention for your curly brackets/indentation and stick with it. You code has all of the following:

• Brackets included, one-lined statement
• Brackets omitted, one-lined statement
• New line after opening bracket, closing bracket on same line.
• New line after opening bracket, closing bracket on different line.
• Un-indented code inside brackets.
• Padding spaces to vertically aligning opening brackets (with multiple different vertical lines being padded to).
• Single space padding opening brackets.
• Blank line before closing bracket.

The Java convention is:

public synchronized boolean add(final Observer<? super T> o) {
   return !this.observers.contains(o) && observers.add(o);
}

It is best to always include curly brackets even when they could be omitted. If you start with an if that does not include brackets and then need to add a second line, forgetting to would have the second line execute unconditionally.

---

It is safer to synchronize on a private instance variable than on the class instance. You have no control over what someone does with the instance of your class. Maybe they think it would be a good idea to use it to synchronize some other code. Now you have two different pieces of code using the same lock instance that might be result in a deadlock because they are not coordinated properly. A private lock variable guarantees that this can't happen.

---

You have to synchronize all accesses to observers. countObservers() could be called while you are adding or removing an observer.

---

Don't create aliases for your methods. It will only lead to confusion if there are multiple ways to do the same things.

---

observersSet is not a Set Don't make the argument name appear as if it is expecting one. If you do want it to be a set (you might since you are checking for duplicates in add()), then change the type from Collection to Set. If you need observers to be called in the order in which they were added, LinkedHashSet satisfies this requirement. If you accept any Collection when creating the instance, there is nothing to prevent the collection from already containing a duplicate.