Commons SCXML
  1. Commons SCXML
  2. SCXML-47

Provide a state machine support class to allow for delegation.

    Details

    • Type: Improvement Improvement
    • Status: Open
    • Priority: Minor Minor
    • Resolution: Unresolved
    • Affects Version/s: 0.6
    • Fix Version/s: 2.0
    • Labels:
      None

      Description

      This is not completely thought out yet, but if folks like the idea I might persue it further.

      I would like to use AbstractStateMachine but cannot extend from it:

      class B extends A /*, AbstractStateMachine */ {
      // copy source from AbstractStateMachine here
      }

      I propose here a StateMachineSupport class that provides for use by subclassing and for use by delegation. The constructors are a little messy, but in the end I have

      public class StateMachineSupport {
      // use by subclassing
      protected StateMachineSupport(...) {
      }

      // use by delegation
      public StateMachineSupport(Object delegator, ...) {
      }

      // ... methods from AbstractStateMachine
      }

      public abstract class AbstractStateMachine extends StateMachineSupport {
      protected AbstractStateMachine(...)

      { super(...); }

      }

      // use by subclassing
      class ConcreteStateMachine extends AbstractStateMachine {
      ConcreteStateMachine()

      { super(..."stopwatch.xml"); }

      public void reset()

      { ... }
      public void running() { ... }

      public void paused()

      { ... }
      public void stopped() { ... }

      }

      // use by delegation
      class DelegatingStateMachine extends SomethingElse {
      StateMachineSupport delegate;

      DelegatingStateMachine()

      { delegate = new StateMachineSupport(this, ..."stopwatch.xml"); }

      public void reset()

      { ... }
      public void running() { ... }

      public void paused()

      { ... }
      public void stopped() { ... }

      }

      StateMachineSupport.java, AbstractStateMachine2.java, StateMachineSupportTest.java, and AbstractStateMachine2Test.java attached.

      1. additional-tests.tar.gz
        2 kB
        Michael Heuer
      2. state-machine-support-src.tar.gz
        3 kB
        Michael Heuer

        Activity

        Hide
        Rahul Akolkar added a comment -

        Sounds useful; the AbstractStateMachine class hasn't been used much by me (it was put together for the stopwatch usecase) but I know others are using it. I believe many improvements are possible – at some point, when we move to JDK 1.5, we can take care of some of this with annotations instead which perhaps may be cleaner.

        To begin, could you fill out an Individual Contributor License Agreement (ICLA)? That is here (the fax number is on the form itself, you can mail it in too if you want):

        http://www.apache.org/licenses/icla.txt

        Its a one-time thing, and is needed for non-trivial contributions, for code provenance and pedigree reasons. Please let us know if you have any questions about the ICLA. Ping this issue when you send it in (unless you choose not to), then we can track its receipt and dig into the details of this improvement. Thanks.

        Show
        Rahul Akolkar added a comment - Sounds useful; the AbstractStateMachine class hasn't been used much by me (it was put together for the stopwatch usecase) but I know others are using it. I believe many improvements are possible – at some point, when we move to JDK 1.5, we can take care of some of this with annotations instead which perhaps may be cleaner. To begin, could you fill out an Individual Contributor License Agreement (ICLA)? That is here (the fax number is on the form itself, you can mail it in too if you want): http://www.apache.org/licenses/icla.txt Its a one-time thing, and is needed for non-trivial contributions, for code provenance and pedigree reasons. Please let us know if you have any questions about the ICLA. Ping this issue when you send it in (unless you choose not to), then we can track its receipt and dig into the details of this improvement. Thanks.
        Hide
        Rahul Akolkar added a comment -

        Tentatively setting fix version to v0.7.

        Show
        Rahul Akolkar added a comment - Tentatively setting fix version to v0.7.
        Hide
        Michael Heuer added a comment -

        ICLA faxed yesterday; original sent in the mail today

        Show
        Michael Heuer added a comment - ICLA faxed yesterday; original sent in the mail today
        Hide
        Rahul Akolkar added a comment -

        Thanks, I can confirm that your CLA has been received (and recorded). I intend to take a look at the attachment and post any comments later in the week.

        Show
        Rahul Akolkar added a comment - Thanks, I can confirm that your CLA has been received (and recorded). I intend to take a look at the attachment and post any comments later in the week.
        Hide
        Rahul Akolkar added a comment -

        Looks good, a few minutiae:

        • Perhaps it is better to have the name elaborate that the support is for authoring classes that model stateful entities (and the operations performed in those states). Do you think the name ClassStateMachineSupport makes more sense? It does to me, but I'm not very good with names.
        • The StateMachineSupport class (new name TBD) needs a ton of Javadoc at the class level so folks can get a hang of whats in there (thanks for the new method Javadocs). Some of your usage code snippets in the opening description of this issue would also help a lot as part of that Javadoc.
        • Code has unused imports (and some other warnings from my IDE about unused stuff that are probably bogus)
        • We should have only one AbstractStateMachine class (the current one can extend StateMachineSupport and we'll be done with that). I don't see what we can do about your constructors being a bit messy comment (I think we'll leave them the way you have them).

        Do you want to make these changes? I can too, but I can't say when.

        Show
        Rahul Akolkar added a comment - Looks good, a few minutiae: Perhaps it is better to have the name elaborate that the support is for authoring classes that model stateful entities (and the operations performed in those states). Do you think the name ClassStateMachineSupport makes more sense? It does to me, but I'm not very good with names. The StateMachineSupport class (new name TBD) needs a ton of Javadoc at the class level so folks can get a hang of whats in there (thanks for the new method Javadocs). Some of your usage code snippets in the opening description of this issue would also help a lot as part of that Javadoc. Code has unused imports (and some other warnings from my IDE about unused stuff that are probably bogus) We should have only one AbstractStateMachine class (the current one can extend StateMachineSupport and we'll be done with that). I don't see what we can do about your constructors being a bit messy comment (I think we'll leave them the way you have them). Do you want to make these changes? I can too, but I can't say when.
        Hide
        Michael Heuer added a comment -

        I have discovered a problem with StateMachineSupport as designed, due to its static reference to stateMachine.

        The following test case will fail:

        public void testMoreThanOneScxmlDocument() throws Exception

        { URL fooScxmlDocument = getClass().getResource("foo.xml"); URL barScxmlDocument = getClass().getResource("bar.xml"); Foo foo = new Foo(fooScxmlDocument); Bar bar = new Bar(barScxmlDocument); assertTrue(fooCalled); // bar's initialstate "bar" never called, since bar's StateMachineSupport has // static reference to stateMachine for foo.xml assertTrue(barCalled); }

        private class Foo {

        private StateMachineSupport delegate;

        public Foo(final URL scxmlDocument)

        { delegate = new StateMachineSupport(this, scxmlDocument); }

        public void foo() { fooCalled = true; }
        }

        private class Bar {

        private StateMachineSupport delegate;

        public Bar(final URL scxmlDocument) { delegate = new StateMachineSupport(this, scxmlDocument); }

        public void bar()

        { barCalled = true; }

        }

        with simple SCXML files

        foo.xml:
        <scxml xmlns="http://www.w3.org/2005/07/scxml" version="1.0" initialstate="foo">
        <state id="foo"/>
        </scxml>

        bar.xml:
        <scxml xmlns="http://www.w3.org/2005/07/scxml" version="1.0" initialstate="bar">
        <state id="bar"/>
        </scxml>

        java.lang.NoSuchMethodException: org.apache.commons.scxml.env.StateMachineSupportTest$Bar.foo()
        at java.lang.Class.getDeclaredMethod(Class.java:1937)
        at org.apache.commons.scxml.env.StateMachineSupport.invoke(StateMachineSupport.java:249)

        I believe the way to make this work is to have StateMachineSupport accept a reference to an instance of SCXML in its constructor or otherwise and to not reuse StateMachineSupport in AbstractStateMachine.

        Sorry for not providing better unit tests in the first place.

        Show
        Michael Heuer added a comment - I have discovered a problem with StateMachineSupport as designed, due to its static reference to stateMachine. The following test case will fail: public void testMoreThanOneScxmlDocument() throws Exception { URL fooScxmlDocument = getClass().getResource("foo.xml"); URL barScxmlDocument = getClass().getResource("bar.xml"); Foo foo = new Foo(fooScxmlDocument); Bar bar = new Bar(barScxmlDocument); assertTrue(fooCalled); // bar's initialstate "bar" never called, since bar's StateMachineSupport has // static reference to stateMachine for foo.xml assertTrue(barCalled); } private class Foo { private StateMachineSupport delegate; public Foo(final URL scxmlDocument) { delegate = new StateMachineSupport(this, scxmlDocument); } public void foo() { fooCalled = true; } } private class Bar { private StateMachineSupport delegate; public Bar(final URL scxmlDocument) { delegate = new StateMachineSupport(this, scxmlDocument); } public void bar() { barCalled = true; } } with simple SCXML files foo.xml: <scxml xmlns="http://www.w3.org/2005/07/scxml" version="1.0" initialstate="foo"> <state id="foo"/> </scxml> bar.xml: <scxml xmlns="http://www.w3.org/2005/07/scxml" version="1.0" initialstate="bar"> <state id="bar"/> </scxml> java.lang.NoSuchMethodException: org.apache.commons.scxml.env.StateMachineSupportTest$Bar.foo() at java.lang.Class.getDeclaredMethod(Class.java:1937) at org.apache.commons.scxml.env.StateMachineSupport.invoke(StateMachineSupport.java:249) I believe the way to make this work is to have StateMachineSupport accept a reference to an instance of SCXML in its constructor or otherwise and to not reuse StateMachineSupport in AbstractStateMachine. Sorry for not providing better unit tests in the first place.
        Hide
        Michael Heuer added a comment -

        failing unit test as described

        Show
        Michael Heuer added a comment - failing unit test as described
        Hide
        Michael Heuer added a comment -

        the same problem exists in AbstractStateMachine itself, see SCXML-48

        Show
        Michael Heuer added a comment - the same problem exists in AbstractStateMachine itself, see SCXML-48
        Hide
        Rahul Akolkar added a comment -

        Indeed, that should be out of the way. Please ping if it isn't. While I was at it, I've added the additional constructors that avoid the recurring parsing cost (by accepting the parsed SCXML instance instead of the URL to the document).

        Show
        Rahul Akolkar added a comment - Indeed, that should be out of the way. Please ping if it isn't. While I was at it, I've added the additional constructors that avoid the recurring parsing cost (by accepting the parsed SCXML instance instead of the URL to the document).

          People

          • Assignee:
            Unassigned
            Reporter:
            Michael Heuer
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:

              Development