Piyush Purang wrote:
The code wasif (!listeners.containsKey(listener)) { listeners.put(listener, listener); and now is synchronized (listeners) { assert (!listeners.contains(listener)); listeners.add (listener); The previous version silently ignored requests to re-register listeners that are already registered. That is the client doesn't have to do any bookkeeping he can register umpteen times and he will be delivered the event just once.
most of the code already did register only once. there were two cases where multiple registration actually uncovered a coding error ;)
imo bookkeeping is not needed in this case. most listener instance are only registered to one entity. and adding / removing itself as a listener is then a matter of life cycle. when a ItemState is created it is added as listener on its overlaid ItemState and when it is destroyed it removes itself as listener.
Now in the new version on the developer's box it will throw an AssertionError and the developer would have been strong-muscled into following the dictum register just once.
as a mentioned before this is a matter of life cycle and no special book keeping is needed.
Most of the times though Listeners that would be registering themselves don't want to hassle theme selves in keeping tabs on how many times did they register. (At least I never programmed my listeners like that. I just registered.)
well, that's very convenient but adds overhead. in our case those listeners are at the very core of jackrabbit and get called a lot of times! Always having to check if a listener is already registered adds an overhead that is not justified for code that is not exposed at an api level.
Hence there is a possibility that listeners on the PE get added multiple-times to the listener queue and receive the same event many times unless we again do some bookkeeping either on our side or listeners do it themselves (Performance will be hit). And no listener expects to be told about the same event more than once; that is something we all can agree on.
I agree, that's why I added the assert statement there ;)
And if you still want to call it a pre-condition then [1] clearly points out don't use Assertions to implement pre-conditions for public methods.
that's because the documentation on that method in the document states that an IllegalArgumentException is thrown. therefore an assertion does not make sense there.
Do you agree with my analysis? I of course don't have the kind of overview that you have of jackrabbit. Maybe my presumption that the outside world will be registering listeners is false. Even in that case I am not 100% convinced about using assertions to fit the role.
listeners on ItemState are not exposed to the outside world. ItemStates are internal data structures in jackrabbit.
How about using a weak hash set?
that would more or less mean we go back to what we had before. HashSet's are usually implemented on top of a HashMap. The problem with the ReferenceMap was that is consumes 4 times the memory than the WeakList we are using now. Additionally the current implementation is a bit faster even though it is implemented as list.
regards marcel
Cheers Piyush [1] http://java.sun.com/j2se/1.4.2/docs/guide/lang/assert.html
