Project: Eclipse_Platform_UI
Bug 132947 [DataBinding] small code style problems in AbstractObservable
Reported: 2006-03-23 01:34:39 | Status: verified fixed

I read new code for JFace data bindings and found couple code style problems in AbstractObservable. Nothing special, but looks not ideal, as it should be Eclipse code. :-) 1. In "addChangeListener". Collection listenerList; if (changeListeners instanceof IChangeListener) { IChangeListener l = (IChangeListener) changeListeners; listenerList = new ArrayList(); listenerList.add(l); } else { listenerList = (Collection) changeListeners; } if (listenerList.size() > 16) { HashSet listenerSet = new HashSet(); listenerSet.addAll(listenerList); changeListeners = listenerList; } Here listenerList declared as collection and initialized with ArrayList. But couple lines below listenerSet declared not as Collection or even Set, but directly as HashSet. 2. In "removeChangeListener". Collection listenerList = (Collection) changeListeners; listenerList.remove(listener); if (listenerList.size() == 0) { changeListeners = null; if (!hasListeners()) { lastListenerRemoved(); } } I think that it is better use "listenerList.isEmpty()" instead of "listenerList.size() == 0". Here I have also two sub-questions: 2.1. may be check that remove() returns "true"? In theory it is possible that collection does not contains passed listener (because of bug in user code?), so rest of code is not required. 2.2. I am not sure, but may be it is good to check for single element is listenerList and rollback to direct listener as changeListeners? Well, most probably no, but just idea...

Files changed in fix:
bundles/org.eclipse.jface.databinding/src/org/eclipse/jface/internal/databinding/internal/viewers/StructuredViewerObservableCollectionWithLabels.java bundles/org.eclipse.jface.databinding/src/org/eclipse/jface/internal/databinding/provisional/observable/AbstractObservable.java bundles/org.eclipse.jface.databinding/src/org/eclipse/jface/internal/databinding/provisional/observable/list/AbstractObservableList.java bundles/org.eclipse.jface.databinding/src/org/eclipse/jface/internal/databinding/provisional/observable/list/ObservableList.java bundles/org.eclipse.jface.databinding/src/org/eclipse/jface/internal/databinding/provisional/observable/mapping/BaseObservableMapping.java bundles/org.eclipse.jface.databinding/src/org/eclipse/jface/internal/databinding/provisional/observable/set/ObservableSet.java bundles/org.eclipse.jface.databinding/src/org/eclipse/jface/internal/databinding/provisional/observable/value/AbstractObservableValue.java bundles/org.eclipse.jface.databinding/src/org/eclipse/jface/internal/databinding/provisional/observable/value/AbstractVetoableValue.java