[Biojava-l] Changing Changeable

[Matthew Pocock]

Hi all,

There is a lot of repetative and error-prone coding required to
implement a changeable class. This tends to involve either inheriting 
from AbstractChangeable, or delegating to ChangeSupport (something that 
has to be done a bit carefly to avoid synchronization problems if lazily 
instantiating the ChangeSupport delegate). Implementing the code in 
ChangeSupport for managing listeners for each case is possible, but 
realy would open the codebase up to a plethora of nasties. Each method 
that alters a changeable property has to put in some realy boring and 
repetative but critical code involving a check for listeners and a 
synchronized block.

I propose that we add an interface PropertyChanger and a method 
ChangeSupport.fireProperty which uses a PropertyChanger and a 
ChangeEvent to update the value of the field, while taking care of 
synchronization and event notification. AbstractChangeable has an 
apropreately modified signature. Here are the changes (with a bit of 
pseudo-code where I can't be bothered to write the whole mess out).

public interface PropertyChanger {
   // update the property
   public void change(ChangeEvent ce);

   // roll back the property
   // - not sure if we want this yet - left out of my initial attempt
   public void unchange(ChangeEvent ce);
}

public class ChangeSupport {
   // drop firePreChange and firePostChange

   ...

   public void fireChange(PropertyChanger pc, ChangeEvent ce)
   throws ChangeVetoException {
     foreach(listener)
       listener.preChange(ce);

     pc.change(ce);

     foreach(listener)
       listener.postChange(ce);
   }

   ...
}

ChangeSupport will allow multiple threads to fire events symultaneously 
and will make sure that the listener list is maintained in a safe 
manner. No external synchronization on ChangeSupport will be needed any 
more, removing many opportunities for bugs.

Here is an example of using a PropertyChanger that captures state in a 
closure. The old method looks like this:

public void train(DistributionTrainerContext dtc, double weight)
throws ChangeVetoException {
   if(!hasListeners())  {
     trainImpl(dtc, weight);
   } else {
     ChangeSupport changeSupport = getChangeSupport(Distribution.WEIGHTS);
     synchronized(changeSupport) {
       ChangeEvent ce = new ChangeEvent(
         SimpleDistribution.this,
         Distribution.WEIGHTS
       );
       changeSupport.firePreChangeEvent(ce);
       trainImpl(dtc, weight);
       changeSupport.firePostChangeEvent(ce);
     }
   }
}

becomes:

public void train(final DistributionTrainerContext dtc, final double weight)
throws ChangeVetoException {
   fireChange(
     new PropertyChanger() {
       public void change(ChangeEvent ce) {
         trainImpl(dtc, weight);
       }
     }
     new ChangeEvent(
       SimpleDistribution.this,
       Distribution.WEIGHTS
     )
   );
}

Admittedly this doesn't cut down the number of lines of code drasticaly, 
but it does greatly reduce the complexity of the code.

Here is another example taken from SimpleEmissionState. The old code is:

public final void setDistribution(Distribution dis)
throws ChangeVetoException {
   if(!hasListeners()) {
     this.dis = dis;
   } else {
     ChangeEvent ce = new ChangeEvent(
       this, EmissionState.DISTRIBUTION,
// actualy a bug - these two are the wrong way arround
       this.dis, dis
     );
     ChangeSupport changeSupport = 
getChangeSupport(EmissionState.DISTRIBUTION);
     synchronized(changeSupport) {
       changeSupport.firePreChangeEvent(ce);
       this.dis = dis;
       changeSupport.firePostChangeEvent(ce);
     }
   }
}

This becomes:

private static final PropertyChanger DISTRIBUTION_CHANGER
   = new PropertyChanger() {
   public void change(ChangeEvent ce) {
     SimpleEmissionState ses = (SimpleEmissionState) ce.getSource();
     ses.dis = (Distribution) ce.getChange();
   }
};

public final void setDistribution(Distribution dis)
throws ChangeVetoException {
   fireChange(
     DISTRIBUTION_CHANGER,
     ChangeEvent ce = new ChangeEvent(
       this, EmissionState.DISTRIBUTION,
       dis, this.dis
     )
   );
}

Obviously, there is no real need to make the changer into a static 
field, but doing it this way feels nice to me, and cuts down on 
object-churn. All the information about what should change is 
encapsulated by the PropertyChanger instance and the ChangeEvent.

This allows us to communicate changes to properties arround the system. 
The ChangeSupport object knows all about the registered listeners and 
the change types that should never be thrown (according to the 
unChanging method). By decoupling the update code from the event fireing 
stuff we can potentialy batch updates. E.g. when committing a set of 
sequences to a database, we can fire all the preChange notifications, 
then do all the updates and finaly fire all the postChange notifications.

If we were to add the unchange method to PropertyChanger, we can 
potentialy roll-back things when they go wrong. In the database example, 
if any one of the fifty sequences being added failed we could make sure 
that the ones added so far are removed, leaving the seqDB in the state 
it was before. None of this can be done with the current system.

This realy would move us a few steps towards much more robust handling 
of data, and allows the vast majority of the overhead to be optimized 
away for common cases (no listeners, one thread only). It becomes much 
easier to write changeable classes. The main cost is that a large number 
of files must be touched. All of these will need to be tested of course. 
Perhaps the JUnit coverage will be increased.

As always, nothing is set in stone, and all comments are welcome. In 
particular, if you have a use-case for the events framework that is not 
realy covered at the moment, now is the time to say.

Matthew