(I have moved this discussion to the arch_at_glassfish.dev.java.net list.)
Hi, Roberto.
I've been thinking more about your suggestion, and although I see some
value in using types to enforce state behavior doing so does not
entirely eliminate the need for state-checking logic...unless I've
missed something.
In your earlier note you actually advocated using different types for
two orthogonal purposes:
1. to segregate the single-class case (in which the calling program
specifies a class to execute) from the true app client case (in which
the program specifies an app client archive). As an example you pointed
out that setting the main class to be run does not make sense for the
single-class case and so there should be compile-time constraints to
enforce that using separate types.
2. to constrain which methods are available to the developer depending
on the state of the app client container (that is, an ACC that has not
yet been started should not be stoppable, for example).
I completely agree with the first purpose and I'm reworking the API to
reflect that.
As for the second purpose, it helps some but in a rather limited way.
In the embedded ACC case, there are three distinct states in which the
ACC can exist from the developer's point of view:
1. instantiated but not yet started - In this state the calling program
can further configure the ACC by invoking any of several methods, and it
can start the ACC but cannot stop it (since it is not running).
2. running - The program has configured the embedded ACC to its liking
and has started it. Further configuration should not be allowed since
the config is used to determine the characteristics of the running ACC.
The calling program can stop the running ACC. It might be an open
question whether the calling program should be able to start another ACC
using the same configuration or not. My own feeling is that, for
simplicity, this should not be permitted.
3. stopped - The program has stopped the running ACC. No further action
should be allowed: no configuation, no starting, no stopping.
Using different types alone for the different states, without including
state-checking logic in the methods, can indeed use the compiler to
prevent the developer from incorrectly invoking actions that should only
be allowed in a *later* phase than the current one. But state-checking
logic is still required to prevent the developer from invoking actions
that should only be allowed *earlier* in the life cycle.
For example (and the type names are only examples):
ACC runnableACC = ACC.newContainer(...);
runnableACC.logger(logger);
// Using different types allows the compiler to prevent the error in the
following commented line
// runnableACC.stop();
RunningACC runningACC = runnableACC.start();
// Using different types does *not* allow the compiler to prevent this
after the ACC has been started:
runnableACC.mainClass("test.MyMain");
The mainClass method implementation could:
-- throw an IllegalStateException (or some other exception),
-- log a warning with or without assigning the new value.
-- return silently after taking no action,
-- assign the new value to the runnableACC's internal private field --
which would be ignored by the RunningACC instance.
The first two - and I would argue that one of these is the right choice
- require state checking in the mainClass method. The last two do not
require state checking but mislead the developer and the user at runtime
by failing to distinguish between an invocation of the method that has
the expected effect and an invocation that does not.
Using the compiler where possible to enforce state-based restrictions
makes sense, but it seems to me that to expose the clearest behavior
requires state-checking logic in the methods of types corresponding to
earlier states in the life cycle.
You did advocate using types to, "as much as possible," prevent illegal
transitions. There's the related issue of preventing or detecting
illegal actions that do not trigger transitions.
I just wanted to make sure I was not missing some aspect of your
suggestion that would rely even more on the compiler, rather than on
runtime checks, to enforce this.
Thanks.
- Tim
Roberto Chinnici wrote:
> Apologies for not cc-ing you, Bill pointed out to me that you may not
> be on the asarch alias.
>
>
> ------------------------------------------------------------------------
>
> Subject:
> Following up on the ACC review
> From:
> Roberto Chinnici <Roberto.Chinnici_at_Sun.COM>
> Date:
> Wed, 04 Feb 2009 15:43:57 -0800
> To:
> asarch <asarch_at_sun.com>
>
> To:
> asarch <asarch_at_sun.com>
>
>
> Hi,
>
> I was looking at the latest AppClientContainer class:
>
> https://glassfish-svn.dev.java.net/source/browse/glassfish-svn/trunk/v3/appclient/client/acc/src/main/java/org/glassfish/appclient/client/acc/AppClientContainer.java?rev=24539&view=markup
>
>
> Although it is more type-safe than what was initially proposed during
> the review (one pager at
> http://wiki.glassfish.java.net/Wiki.jsp?page=gfv3-appclientcontainer-onepager),
> it doesn't fully reflect my suggestion for making it more "fluent". So
> I thought I'd expand the comments I made during the meeting here.
>
> In the version in svn, one would write:
>
> AppClientContainer acc = AppClientContainer.newContainer(host,
> port, client);
> acc.setLogger(logger);
> acc.setMainClass(mainClassName);
> acc.run();
>
> (...later...)
>
> acc.stop();
>
> Some of these methods will throw exceptions is called at the wrong time.
>
> Also, the setMainClass method is optional. Here's the default rule:
> "the default is the class specified by the Main-Class attribute of the
> client's manifest if the client file is a client JAR or directory, and
> the .class file itself if the calling program passed a .class file as
> the client file".
>
> Not very easy to understand, IMO.
>
> My suggestion during the meeting was to use types to represent the
> state transitions, starting with the creation of the container, so
> that only valid methods could ever be invoked.
>
> E.g.
>
> AppClientContainer acc = AppClientContainer.newContainer(host,
> port, client);
> RunningAppClientContainer racc =
> acc.logger(logger).mainClass(mainClassName).run();
> racc.stop();
>
> The state transition from created/being configured to running would
> involve returning a RunningAppClientContainer from the run method.
> Clearly, an AppClientContainer would not have a stop method at all.
> Not also that the setters now return the receiver, as in builder
> methods, to allow cascading.
>
> For setMainClass, the thing to avoid is to let somebody call
> setMainClass when they have passed in a .class file as the client
> file, since the intent of the latter is to be the main class.
>
> The solution here is to have different newContainer methods, with
> well-chosen names, returning objects of different types. The
> setMainClass methods, renamed to mainClass to allow for cascading the
> configuration (as shown in my example), would only be on one of the
> branches.
>
> Example:
>
> OneClassAppClientContainer acc =
> AppClientContainer.newContainerFromClass(host, port, clientMainClass);
> RunningAppClientContainer racc = acc.logger(logger).run();
> racc.stop();
>
> OneClassAppClientContainer doesn't have a setMainClass method, whereas
> the AppClientContainer from the earlier example has such a method.
>
> One could also debate whether
> OneClassAppClientContainer acc =
> AppClientContainer.newContainerFromClass(host, port, clientMainClass);
> would not be better written as
> OneClassAppClientContainer acc =
> OneClassAppClientContainer.newContainer(host, port, clientMainClass);
> i.e. how to distribute the factory methods.
> I tend to think that centralizing them is better, because it gives
> developers a single entry point (including in the javadocs).
>
> (I'm not really sure if setMainClass is the best example of the point
> I'm trying to make, by the way.)
>
> In general I think that, in the presence of state transitions, the
> methods available on a type should guide the developer, preventing him
> as much as possible from attempting illegal transitions.
>
> --Roberto
>