On Sun, Aug 12, 2001 at 09:54:37PM +1000, Peter Donald wrote:
> On Sun, 12 Aug 2001 21:06, Jeff Turner wrote:
> > A question, mainly for Pete,
> >
> > Enum and ValuedEnum both have public constructors:
> >
> > public Enum( final String name );
> > public Enum( final String name, final Map map );
> > public ValuedEnum( final String name, final int value );
> > public ValuedEnum( final String name, final int value, final Map map );
> >
> > Because they're public, any class can add more Enum items at any time,
> > so the Enum is not type-safe at all. Possibly a security hazard if
> > anyone was relying on subclasses' type-safety.
> >
> > Since the only time they should ever be called is within a subclass,
> > would it be better to make them protected? This technically breaks
> > backwards-compatibility, but only where the class is being incorrectly
> > used.
> 
> It used to be protected at one stage ... can't seem to recall why it was made 
> public. Is there any code in Avalon that it would break?

Er.. yes, actually.

Two cases in Phoenix:

src/java/org/apache/avalon/framework/camelot/State.java
src/java/org/apache/avalon/phoenix/components/phases/BlockDAG.java

In both cases, moving the constructor calls from outside to inside is
pretty easy, and the code looks just as good. I don't think these
examples represent a new use-case for non-typesafe Enums.

Still, it's a backwards-incompatible change. Should we create a CVS
branch "avalon-5_0" for things like this?

Diffs for the avalon and phoenix changes attached.

--Jeff

> Cheers,
> 
> Pete
> 
> *-----------------------------------------------------*
> | "Faced with the choice between changing one's mind, |
> | and proving that there is no need to do so - almost |
> | everyone gets busy on the proof."                   |
> |              - John Kenneth Galbraith               |
> *-----------------------------------------------------*
> 
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [EMAIL PROTECTED]
> For additional commands, e-mail: [EMAIL PROTECTED]
Index: src/java/org/apache/avalon/framework/Enum.java
===================================================================
RCS file: 
/home/cvs/jakarta-avalon/src/java/org/apache/avalon/framework/Enum.java,v
retrieving revision 1.3
diff -u -r1.3 Enum.java
--- src/java/org/apache/avalon/framework/Enum.java      2001/08/12 10:48:01     
1.3
+++ src/java/org/apache/avalon/framework/Enum.java      2001/08/12 13:05:31
@@ -60,7 +60,7 @@
      * Constructor to add a new named item.
      * @param name Name of the item.
      */
-    public Enum( final String name )
+    protected Enum( final String name )
     {
         this( name, null );
     }
@@ -71,7 +71,7 @@
      * @param map A <code>Map</code>, to which will be added a pointer to the 
newly constructed
      * object.
      */
-    public Enum( final String name, final Map map )
+    protected Enum( final String name, final Map map )
     {
         m_name = name;
         if( null != map )
Index: src/java/org/apache/avalon/framework/ValuedEnum.java
===================================================================
RCS file: 
/home/cvs/jakarta-avalon/src/java/org/apache/avalon/framework/ValuedEnum.java,v
retrieving revision 1.5
diff -u -r1.5 ValuedEnum.java
--- src/java/org/apache/avalon/framework/ValuedEnum.java        2001/08/12 
10:48:01     1.5
+++ src/java/org/apache/avalon/framework/ValuedEnum.java        2001/08/12 
13:05:31
@@ -69,7 +69,7 @@
      * @param name the name of enum item.
      * @param value the value of enum item.
      */
-    public ValuedEnum( final String name, final int value )
+    protected ValuedEnum( final String name, final int value )
     {
         this( name, value, null );
     }
@@ -82,7 +82,7 @@
      * @param value the value of enum item.
      * @param map the <code>Map</code> to add enum item to. 
      */
-    public ValuedEnum( final String name, final int value, final Map map )
+    protected ValuedEnum( final String name, final int value, final Map map )
     {
         super( name, map );
         m_value = value;
Index: src/java/org/apache/avalon/framework/camelot/State.java
===================================================================
RCS file: 
/home/cvspublic/jakarta-avalon-phoenix/src/java/org/apache/avalon/framework/camelot/State.java,v
retrieving revision 1.1
diff -u -r1.1 State.java
--- src/java/org/apache/avalon/framework/camelot/State.java     2001/04/27 
05:03:30     1.1
+++ src/java/org/apache/avalon/framework/camelot/State.java     2001/08/12 
12:51:31
@@ -14,10 +14,17 @@
  *
  * @author <a href="mailto:[EMAIL PROTECTED]">Peter Donald</a>
  */
-public class State
+public final class State
     extends ValuedEnum
 {
-    public State( final String name, final int value )
+
+    //A list of constants representing phases in Blocks lifecycle.
+    //Each phase is made up of a number of stages.
+    public final static State  BASE       = new State( "BASE", 0 );
+    public final static State  STARTEDUP  = new State( "STARTEDUP", 10 );
+    public final static State  SHUTDOWN   = new State( "SHUTDOWN", 20 );
+
+    private State( final String name, final int value )
     {
         super( name, value );
     }
Index: 
src/java/org/apache/avalon/phoenix/components/application/DefaultServerApplication.java
===================================================================
RCS file: 
/home/cvspublic/jakarta-avalon-phoenix/src/java/org/apache/avalon/phoenix/components/application/DefaultServerApplication.java,v
retrieving revision 1.4
diff -u -r1.4 DefaultServerApplication.java
--- 
src/java/org/apache/avalon/phoenix/components/application/DefaultServerApplication.java
     2001/07/17 07:54:35     1.4
+++ 
src/java/org/apache/avalon/phoenix/components/application/DefaultServerApplication.java
     2001/08/12 12:51:33
@@ -164,13 +164,13 @@
     {
         PhaseEntry entry = new PhaseEntry();
         entry.m_visitor = new StartupPhase();
-        entry.m_traversal = BlockDAG.FORWARD;
+        entry.m_traversal = BlockDAG.Traversal.FORWARD;
         m_phases.put( "startup", entry );
         setupComponent( entry.m_visitor, "StartupPhase" );
 
         entry = new PhaseEntry();
         entry.m_visitor = new ShutdownPhase();
-        entry.m_traversal = BlockDAG.REVERSE;
+        entry.m_traversal = BlockDAG.Traversal.REVERSE;
         m_phases.put( "shutdown", entry );
         setupComponent( entry.m_visitor, "ShutdownPhase" );
     }
Index: src/java/org/apache/avalon/phoenix/components/kapi/BlockEntry.java
===================================================================
RCS file: 
/home/cvspublic/jakarta-avalon-phoenix/src/java/org/apache/avalon/phoenix/components/kapi/BlockEntry.java,v
retrieving revision 1.1
diff -u -r1.1 BlockEntry.java
--- src/java/org/apache/avalon/phoenix/components/kapi/BlockEntry.java  
2001/07/17 07:05:45     1.1
+++ src/java/org/apache/avalon/phoenix/components/kapi/BlockEntry.java  
2001/08/12 12:51:34
@@ -21,12 +21,6 @@
 public class BlockEntry
     extends Entry
 {
-    //A list of constants representing phases in Blocks lifecycle.
-    //Each phase is made up of a number of stages.
-    public final static State  BASE       = new State( "BASE", 0 );
-    public final static State  STARTEDUP  = new State( "STARTEDUP", 10 );
-    public final static State  SHUTDOWN   = new State( "SHUTDOWN", 20 );
-
     private final RoleEntry[]   m_roleEntrys;
 
     private final String        m_name;
@@ -38,7 +32,7 @@
         m_name = name;
         m_roleEntrys = roleEntrys;
         setLocator( locator );
-        setState( BASE );
+        setState( State.BASE );
     }
 
     public String getName()
Index: src/java/org/apache/avalon/phoenix/components/phases/BlockDAG.java
===================================================================
RCS file: 
/home/cvspublic/jakarta-avalon-phoenix/src/java/org/apache/avalon/phoenix/components/phases/BlockDAG.java,v
retrieving revision 1.3
diff -u -r1.3 BlockDAG.java
--- src/java/org/apache/avalon/phoenix/components/phases/BlockDAG.java  
2001/07/17 12:23:51     1.3
+++ src/java/org/apache/avalon/phoenix/components/phases/BlockDAG.java  
2001/08/12 12:51:35
@@ -37,13 +37,13 @@
     private static final Resources REZ =
         ResourceManager.getPackageResources( BlockDAG.class );
 
-    public final static Traversal  FORWARD     = new Traversal( "FORWARD" );
-    public final static Traversal  REVERSE     = new Traversal( "REVERSE" );
-    public final static Traversal  LINEAR      = new Traversal( "LINEAR" );
-
     public final static class Traversal
         extends Enum
     {
+           public final static Traversal  FORWARD     = new Traversal( 
"FORWARD" );
+               public final static Traversal  REVERSE     = new Traversal( 
"REVERSE" );
+       public final static Traversal  LINEAR      = new Traversal( "LINEAR" );
+
         private Traversal( final String name )
         {
             super( name );
@@ -116,7 +116,7 @@
             final RoleEntry roleEntry = entry.getRoleEntry( role );
             final String dependencyName = roleEntry.getName();
             final BlockEntry dependency = getBlockEntry( dependencyName );
-            visitBlock( dependencyName, dependency, visitor, FORWARD, 
completed );
+            visitBlock( dependencyName, dependency, visitor, 
Traversal.FORWARD, completed );
         }
     }
 
@@ -162,7 +162,7 @@
                     }
 
                     //finally try to traverse block
-                    visitBlock( blockName, entry, visitor, REVERSE, completed 
);
+                    visitBlock( blockName, entry, visitor, Traversal.REVERSE, 
completed );
                 }
             }
         }
@@ -179,11 +179,11 @@
         if( completed.contains( name ) ) return;
         completed.add( name );
 
-        if( FORWARD == traversal )
+        if( Traversal.FORWARD == traversal )
         {
             visitDependencies( name, entry, visitor, completed );
         }
-        else if( REVERSE == traversal )
+        else if( Traversal.REVERSE == traversal )
         {
             visitReverseDependencies( name, visitor, completed );
         }
Index: src/java/org/apache/avalon/phoenix/components/phases/ShutdownPhase.java
===================================================================
RCS file: 
/home/cvspublic/jakarta-avalon-phoenix/src/java/org/apache/avalon/phoenix/components/phases/ShutdownPhase.java,v
retrieving revision 1.3
diff -u -r1.3 ShutdownPhase.java
--- src/java/org/apache/avalon/phoenix/components/phases/ShutdownPhase.java     
2001/07/17 12:23:51     1.3
+++ src/java/org/apache/avalon/phoenix/components/phases/ShutdownPhase.java     
2001/08/12 12:51:36
@@ -11,6 +11,7 @@
 import org.apache.avalon.framework.activity.Disposable;
 import org.apache.avalon.framework.activity.Startable;
 import org.apache.avalon.framework.camelot.Container;
+import org.apache.avalon.framework.camelot.State;
 import org.apache.avalon.framework.component.ComponentException;
 import org.apache.avalon.framework.component.ComponentManager;
 import org.apache.avalon.framework.component.Composable;
@@ -51,7 +52,7 @@
     public void visitBlock( final String name, final BlockEntry entry )
         throws Exception
     {
-        if( entry.getState() != BlockEntry.STARTEDUP ) return;
+        if( entry.getState() != State.STARTEDUP ) return;
 
         if( getLogger().isInfoEnabled() )
         {
@@ -111,7 +112,7 @@
         //Destruction stage
         getLogger().debug( REZ.getString( "shutdown.notice.destroy.pre" ) );
         entry.setInstance( null );
-        entry.setState( BlockEntry.SHUTDOWN );
+        entry.setState( State.SHUTDOWN );
         getLogger().debug( REZ.getString( "shutdown.notice.destroy.success" ) 
);
 
         try
Index: src/java/org/apache/avalon/phoenix/components/phases/StartupPhase.java
===================================================================
RCS file: 
/home/cvspublic/jakarta-avalon-phoenix/src/java/org/apache/avalon/phoenix/components/phases/StartupPhase.java,v
retrieving revision 1.4
diff -u -r1.4 StartupPhase.java
--- src/java/org/apache/avalon/phoenix/components/phases/StartupPhase.java      
2001/07/17 13:24:00     1.4
+++ src/java/org/apache/avalon/phoenix/components/phases/StartupPhase.java      
2001/08/12 12:51:38
@@ -13,6 +13,7 @@
 import org.apache.avalon.framework.CascadingException;
 import org.apache.avalon.framework.activity.Initializable;
 import org.apache.avalon.framework.activity.Startable;
+import org.apache.avalon.framework.camelot.State;
 import org.apache.avalon.framework.camelot.Container;
 import org.apache.avalon.framework.camelot.ContainerException;
 import org.apache.avalon.framework.camelot.Entry;
@@ -98,7 +99,7 @@
     public void visitBlock( final String name, final BlockEntry entry )
         throws Exception
     {
-        if( entry.getState() != BlockEntry.BASE &&
+        if( entry.getState() != State.BASE &&
             null != entry.getState() ) return;
 
         if( getLogger().isInfoEnabled() )
@@ -181,7 +182,7 @@
                 getLogger().debug( REZ.getString( 
"startup.notice.start.success" ) );
             }
 
-            entry.setState( BlockEntry.STARTEDUP );
+            entry.setState( State.STARTEDUP );
         }
         catch( final Exception e )
         {

---------------------------------------------------------------------
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]

Reply via email to