Name: krC82822			Date: 06/25/2001


java version "1.4.0-beta"
Java(TM) 2 Runtime Environment, Standard Edition (build 1.4.0-beta-b65)
Java HotSpot(TM) Client VM (build 1.4.0-beta-b65, mixed mode)

The design of the classes related to the new persistence delegation
seems counterproductive and does not match specification at
some points. Some methods have undocumented side effects.

The Encoder class is intended to be a base class for different
Encoder implementations but is mostly designed as if it should never
be subclassed and implemented as if XMLEncoder was the only possible
subclass and if it were a singleton design.

Most important points are:

- Encoder should be abstract
- Encoder should provide abstract methods for the implementors
  e.g., a method which is called after writeStatement has
  processed it and has created the nessessary expressions
- The logic of maintaining a statement list and value to expression
  mapping as well as looking up which expressions are really needed
  by the written statements should be provided by the Encoder not
  implemented in XMLEncoder and re-implemented by all other Encoder
  implementations
  90% of the XMLEncoder code is maintaining these informations instead
  of dealing with XML encoding what the class actually is for
- getPersistenceDelegate and setPersistenceDelegate are instance
  methods but have static effect. These should be either static methods
  or have an effect per instance
- getValue(Expression) should be protected not package-private
- clear() should be protected not package-private
- XMLEncoder should not call any package-private methods.
  Then it would better match it's role of a reference
  implementation of an Encoder and the design flaws would
  be detected earlier
- The constructor of XMLEncoder has global side-effects.
  BTW the called static method setCaching(boolean) is replacing the cache
  by a new HashMap every time it is set to true, even if it was true before
- The flush() method of XMLEncoder has global side-effects
- The toString() method of Statement has global side-effects
  The name assigned to the target of the statement is stored in a global
  map. This map is cleared in the flush() method of XMLEncoder. This means:
  - calling flush() on a XMLEncoder causes other Encoders to output
    inconsistent names if Statement.toString() is used.
  - if other Encoder implementations are using the toString() method of
    Statement the target objects are never garbage-collected
- There might be more side-effects

It seems as if the whole code should be better completly rewritten.

The following code can be used to indicate some of the above points.


import java.lang.reflect.*;
import java.beans.*;
import java.io.ByteArrayOutputStream;
import java.util.HashMap;

class TestCase {
  static void main(String[] arg) {
    try {
      System.out.print("Is getPersistenceDelegate DECLARED static ? ");
      Class[] argType=new Class[] {Class.class};
      Method m=Encoder.class.getMethod("getPersistenceDelegate",argType);
      System.out.println(Modifier.isStatic(m.getModifiers()));

      System.out.print("Is the EFFECT of set/getPersistenceDelegate static ? ");
      Encoder e1=new XMLEncoder(new ByteArrayOutputStream()),
              e2=new XMLEncoder(new ByteArrayOutputStream());
      PersistenceDelegate pd=new DefaultPersistenceDelegate();
      e1.setPersistenceDelegate(TestCase.class, pd);
      System.out.println(e2.getPersistenceDelegate(TestCase.class)==pd);

      System.out.println("Writing an object to a debug Encoder");
      e1=new Encoder() {
        public void writeExpression(Expression ex) {
          super.writeExpression(ex);
          System.out.println("  Expression: "+ex);
        }
      };
      Object testObj=new java.awt.Point(4,2);
      e1.getPersistenceDelegate(testObj.getClass()).writeObject(testObj,e1);

      System.out.print("now, are there DANGLING REFERENCES avoiding gc ? ");
      Class nameCl=Class.forName("java.beans.NameGenerator");
      Field nameFd=nameCl.getDeclaredField("valueToName");
      nameFd.setAccessible(true);
      HashMap names=(HashMap)nameFd.get(null);
      System.out.println(!names.isEmpty());
      System.out.println("  "+names.size()+" objects");

      if(!names.isEmpty()) {
        System.out.print("Does XMLEncoder.flush() have a GLOBAL SIDE-EFFECT"
          +" to these references ? ");
        ((XMLEncoder)e2).flush();
        HashMap names2=(HashMap)nameFd.get(null);
        System.out.println(!names2.equals(names));
        System.out.println("  "+(names2.isEmpty()?"now":"still")
          +" contains "+names2.size()+" objects");
      }
    }
    catch(Exception ex) {
      System.out.println("\nException occured: "+ex);
      System.out.println("Note that this TestCase is specific to version "
        +"1.4.0-beta-b65");
    }
  }
}
(Review ID: 127273) 
======================================================================