0
\$\begingroup\$

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.

\$\endgroup\$
3
\$\begingroup\$

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

\$\endgroup\$
  • \$\begingroup\$ 1. I'm really sorry I had to mention there is a static <F> Observable<F> getInstance(Collection<...> reg) (not complete yet). It Allows using any Collection Impl without inheritance, and so a Set is not used. I didn't know how to copy toNotify as it't not concrete (unmodifiableCollection will do I guess). If this is good, many problems you mentioned are already solved. 2. Is it fully up to me deciding to create an extendable class? There's no rule? \$\endgroup\$ – hkoosha Jan 9 '15 at 0:14
  • 1
    \$\begingroup\$ A set's addAll also accepts Collection. I don't exactly see how that's a hindrance. On a related note I don't understand what you want to express with your second point \$\endgroup\$ – Vogel612 Jan 9 '15 at 0:17
  • \$\begingroup\$ Oh and one more thought: Instead of all the hassle about passing in a Collection, I make this class abstract (or an interface, if I managed to do it!) and create other concrete classes: SetBasedObservable, ListBasedObservable, OrderPreservedObservable of-course with better names. But still I'm confused about this: Should each of this classes be final? or I should let others extend it? (Assuming I'd design it to be extended without danger of breaking it). \$\endgroup\$ – hkoosha Jan 9 '15 at 0:18
  • 1
    \$\begingroup\$ The extensibility is completely yours to decide. I'd suggest extracting the public common methods to an interface and make the current Implementation an AbstractObservable which is similar to the way this is handled at other places in java. On a related note, it's IMO easier to work with Collection when accepting input from outside \$\endgroup\$ – Vogel612 Jan 9 '15 at 0:22
  • 1
    \$\begingroup\$ Why would they want to register the same Observer twice? Limit users because some of them can't handle their own mistakes. Limiting users in programming often has to do with protecting them from their own stupidity \$\endgroup\$ – Vogel612 Jan 9 '15 at 0:24

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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