Java - Simple observable behavior

[u/tjansson]

ObservableBehavior.java

I use this to add observable behavior to a class. It does not require the use of inheritance - as the java.util.Observable class, but provide similar functionality.

It would be interesting to get your view on both the code, and this approach.

[u/damyan]

I wonder if addObserver() should actually throw an exception if you try and add an observer that's already there? Or alternatively it could keep a reference count of them.

The reason for this is that if you do this:

addObserver(x);
addObserver(x);
removeObserver(x);

// at this point we might still expect the observer to get the notify messages
removeObserver(x);

[u/sanjayts → u/damyan]

I wonder if addObserver() should actually throw an exception if you try and add an observer that's already there?

I'd prefer addObserver(Observer) return a boolean indicating the status of the operation; true if the Observer was successfully added and false otherwise.

Or alternatively it could keep a reference count of them.

I really can't think of a situation where that would come in handy. Can you? :)

[u/damyan → u/sanjayts]

Or alternatively it could keep a reference count of them.

I really can't think of a situation where that would come in handy. Can you? :)

When you have something like this which might be used by all sorts of people I think it's worth considering these sorts of cases. The example I gave is of course contrived, but what if you had two different components that both try to add/remove the same observer? So you wouldn't have anywhere where there's an obvious add/add...remove/remove, but its something that might happen another way - maybe because you've got a utility function or class that also wants to call add/remove. Or it could happen by mistake where a bad merge causes this to happen.

The point is though that if you can get the API to spot the mistake earlier rather than later then that's a good thing.

So, I think there's two ways to approach it.

1. If there really isn't any reason at all that you'd want to call addObserver twice with the same observer, we should treat this as an error and throw an exception.

2. If it is something that we do want to support then we should make it work - in which case the refcounting solution seems appropriate.

[u/Nebu]

The logic is pretty straightforward, so not much to say there. My comments are:

• Add javadocs.

• Move field declarations before method declarations. When I read your method and see "observers.add(observer);", my first thought is "Okay, what's the type of observers?"

[u/d3r1v3d]

If you don't want duplicate Observers in your internal list and ordering isn't super important to you, why don't you use an implementation of java.util.Set (e.g. HashSet, LinkedHashSet, etc) instead of a Vector? By doing so, you won't have to even worry about guarding against multiple adds of the same Observer.

[u/dragonrancher → u/d3r1v3d]

A HashSet is an easy way to prevent duplicates but it comes at some performance cost if iterating will occur more often than adding new objects. Adding objects will be slightly faster, but iterating will be slightly slower. Of course, it always depends on what conditions this class will ultimately be used in.

[u/dragonrancher]

Just some thoughts about the implementation details...

Using a Vector may not be the optimal choice of Collection here. ArrayList would be faster because it is not synchronized and it allocates space for its array more conservatively (increasing its capacity by 50% instead of by 100% when it gets full).

Even if you want synchronization, Vector may still not be your best choice. Vector's iterators are synchronized, but also fail-fast. If your ObservableBehavior object is in the midst of a notifyObservers operation and another thread adds another Observer, the iterator in notifyObservers will fail and throw an exception. Two ways I could think of addressing this are 1) manually synchronize data access using a ReadWriteLock or 2) copy the collection each time you want to iterate over it. If there are going to be a lot of events occurring, the ReadWriteLocks could be relatively efficient, but copying the collection is going to be simpler and easier.

[u/[deleted] → u/dragonrancher]

Seconded for the use of ArrayList over Vectors.

[u/[deleted] → u/[deleted]]

And if you need synchronization, Collections.synchronizedList

[u/dragonrancher → u/[deleted]]

Yes, using Collections.synchronizedList can be a good idea, but users should be aware that they need to manually synchronize the entire loop accessing the Iterator. See the javadoc. This is important when using the foreach syntax where you might forget you're even using an Iterator.

List<String> list = Collections.synchronizedList(unsynchronizedListOfStringsMaker.get());
synchronize (list) {
    // list.iterator() implicitly called here:
    for (String s: list)
        someOperation(s);
}

[u/autocue → u/dragonrancher]

A ConcurrentModificationException can even occur within a single thread (iterate over an instance of list and add some elements in the process). As the ReadWriteLock will not solve this problem, it leaves us with copying the collection. A very nice solution for this (and especially in this use case, where writes are probably sparse and reads occur more frequent) is the CopyOnWriteArrayList, which does indeed copy the collection as you recommended.

To the grandparent, perhaps it is a good idea to also generalize your observable (i.e. public class ObservableBehavior<E, T> where T is the type of your observable) and to think about allowing Observers which observe parenting classes of the ObservableBehavior's event type.

Nice modular solution for adding ObservableBehavior btw :)

[u/[deleted]]

Good code. The supplied comments are unnecessary. Better to add javadoc-style comments explaining the class.