Does Java need an Observable object with generics?

Question

This is the follow-up question from here.

This is an Observable class similar to java.util.Observable. The difference is that it uses generics to avoid casts.

There are two questions:

1. Is it really worth the effort? or it's just making things more complex than needed?
2. Should this class be declared final, or is it ok to be subclassed? java.util.Observable is not final.

package cc.koosha.silkroad.lang;    
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> {
    public interface Observer<T> {
        public void update(Observable observable, T args);
    }

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

    /**
     * @TODO should this be private and the class final?.
     */
    protected Observable(final Collection<Observer<? super T>> registry) {
        this.registry = registry;
    }

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

    // @TODO Should o be checked for null?
    public void addObserver(final Observer<? super T> o) {
        synchronized (this.registry) {
            if (!this.registry.contains(o))
                this.registry.add(o);
        }
    }

    protected void update(final T args) {
        ArrayList<Observer<? super T>> toNotify;

        synchronized (this.registry) {
            if (!changed)
                return;

            toNotify = new ArrayList<Observer<? super T>>(this.registry);
            this.changed = false;
        }
        // Look at OpenJDK to see why a new Collection is used.
        for (Observer<? super T> o : toNotify)
            o.update(this, args);
    }

    public void notifyObservers(final T args) {
        this.update(args);
    }

Methods deleteObserver, deleteObservers, setChanged, clearChanged, hasChanged, countObservers, notifyObservers are removed for brevity.

The rest of the code is at GitHub.

Answer 1

A few observations.

Your choice of Collection as type for registry and all other things point at it that you want this class to be extended. If it weren't so a few desing choices would have been different.

I still want to review the code as if it were a closed class. Because if it weren't I'd suggest you throw that all away and think about why able things very often are interfaces. Don't make the same design mistakes...

Your registry is overly Generic. Not in choice of generics, but in choice of Interface...

I'd have expected at least a Set instead of Collection. Also your code could greatly benefit from eagerly initializing it. For one it allows you to add a no-argument constructor easily, and secondly it's more conventional.

Let's look at getInstance for a moment.
You create an empty collection to be set as your registry. This is boilerplate code when extending your Observable.
Additionally it makes the behavior of your observable concerning references somewhat unconventional. I'd have expected:

 ... Observable (Collection <Observer <? super T>> registry) {
    this.registry.addAll(registry);
}

And here's already my next problem... why is your constructor private? I think it would be beneficial to allow me as programmer to instantiate Observables myself ;)

And last but not least toNotify in your update method is never written to again. I suggest you use a unmodifiableCollection instead of copying your registry into an ArrayList to clearly express what you actually want