On 1/14/06, Jörg Schaible <[EMAIL PROTECTED]> wrote:
> Hi folks,
>
> reviewing c-id I found more issues to resolve before releasing 1.0 final.
> Since I already reported different complaints, I create here a summary of
> anything already reported and all new issues.
>
> 1/ Concept of IdentifierGeneratorFactory
>
> The IdentifierGeneratorFactory provides a easy-to-use concept for casual use
> of id generators. Three elements are part of it:
>
> abstract class IdentifierGeneratorFactory - defines the generator types
> class DefaultIdentifierGeneratorFactory - delivers implementations
> class IdentifierUtils - easy id generation using the default factory
>
> the basic idea is, that a user can implement an own factory and provide it
> with a system factory and it is automatically used as default in the
> IdentifierUtils - so far so good.
>
> The bad part in the concept is, that we have a variety of generator
> implementations and all of them are quite different in their
> initialization. Therefore has the IdentifierGeneratorFactory not only three
> simple methods to generate the default generator for each (currently
> supported) id type (String, Long and UUID), but has a variety of methods
> tailored exactly for the ctors of the different generator implementations,
> which makes this "factory" quite obscure.
>
Agreed. This is a consequence of the growth in the number of
different generator types.
> Modifying a class with every new implementations is for me a kind of "code
> smell". And the current code already gives evidence of this, since we have
> new generator implementations, that cannot be accessed/initialized with the
> factory. Additionally some methods have been added to support newer
> generator implementations, but the appropriate methods to make use of the
> generated ids are missing in the IdentifierUtils.
>
Agreed again. The idea was to allow pluggable implemenations of the
different generator types, but as the number of types increases, this
gets unweildy.
> So we either complete those 3 classes for all generator implementations or
> tailor them to a useful subset for the casual user:
>
> interface IdentifierGeneratorFactory {
> LongIdentifierGenerator longIDentifierGenerator();
> StringIdentifierGenerator stringIDentifierGenerator();
> UUIDIdentifierGenerator uuidIDentifierGenerator();
> }
But then you lose the distinction among the different subtypes. The
question is, do we care about making the implementation of, say
prefixAlphanumericGenerator pluggable (i.e., would anyone ever really
want to provide a different factory implementation that returned a
generator different from PrefixAlphaNumericGenerator from the one we
provide? And if so, is the current setup the best way to support
that. Answer is likely "no" to at least the second question.
>
> The question is, if a user *normally* requires in one application different
> UUID genenerators, string generators or long generators? In all special
> cases the generator instance can be generated directly or by an appropriate
> environment (standard use case in IoC containers, where the implementation
> and their initialization is configured anyway).
>
> Proposal: Use as default a VersionFourGenerator, SessionIdGenerator and
> LongGenerator. If he has the need for a different generator or a different
> initialization, he can still provide easily an own factory and set the
> system property.
It might actually be better to dispense with
IdentifierGeneratorFactory altogether, leaving the utils class in
place, but having it directly instantiate provided impls. This would
also allow us to drop the [discovery] dependency (with some additional
work on uuid if we decidt to include this in the release - see below).
>
> 2/ Exception handling for UUID 1 generator state
>
> Currently c-id cannot be compiled with JDK 1.3, since it uses the ctor of
> the RuntimeException with a causing Throwable. While this can easily
> removed, the concepts leading to the situation must be reconsidered.
>
> The UUID1 generation supports several implementations to persist the state
> of such an generator. The State interface has a load and store method that
> throw both just an arbitrary Exception, mainly because the underlying
> implementation may have any kind of problems reading or storing the state
> (XML parser, IO, DB, ...). The funny part is, that such an exception is
> wrapped by the RuntimeEx from above and from the user's PoV he might get a
> bad surprise calling nextIdentifier() on a UUID 1 generator ...
>
> Proposal: Introduce a StatePerstistenceException derived from RTEx, that
> might be thrown from the State implementations and document this at the
> proper places in javadoc.
>
> 3/ xxxStateImpl classes
>
> The javadoc of ReadOnlyResourceStateImpl implies that the implementation
> tries to discover (well, use c-discovery) to load a file "uuid-[n].conf".
> In reality it expects a system property (wrong name documented) set to a
> file name that can be loaded as resource. All unit tests use a file
> "uuid1.state", that is only available during the tests.
>
> I am just wondering, if a default is simply missing or left by intension.
>
> Also the derived ReadWriteFileStateImpl tries to write into the URL of the
> resource ... which will fail badly if the file is within a jar.
>
> 4/ Copied c-codec classes in official API
>
> To remove a dependency to c-codec the digest and hex utilities have been
> copied to c-id, but they are now publicly available in the o.a.c.id
> namespace.
>
> Martin already proposed to move them to a package o.a.c.id.internal and
> provide an appropriate package.html. As alternative we could try to make
> them package accessible only and remove any unused functionality (they have
> bad coverage reports because we only use view methods).
>
> 5/ Unit tests
>
> A lot of unit tests in the c-id package have own suite methods and set an
> own name for the test. While there are situations, where the usage of a
> suite can be necessary, here it is definitely not. This not only results in
> an inconsistent unit test report, but also prevents, that e.g. in Eclipse I
> can simply double click the test to get to the appropriate code.
>
> Proposal: Remove unnecessary suite methods and TestCase ctors taking a name.
>
> 6/ UUID version 2 + 3 + 5
>
> Looking into the code if UUID.java I am not sure if 2+3 is already
> supported. Some implementation details in nameUUIDFromString seem to
> support also these versions, but there is no appropriate
> VersionTwoGenerator or VersionThreeGenerator. What is the state of it?
>
> 7/ Exception signatures
>
> Some methods have dubious exception signatures. Very bad is e.g. the ctor of
> VersionOneGenerator. It claims to throw 4 different exceptions, but none of
> them is actually thrown. Even worse, those exceptions are propagated to the
> signature of a lot of callers ... up to the factory of issue 1.
>
> While it is clear that I can clean this up, what is the common sense for
> exception handling in c-id? Better wrap them up in an own RTEx derived
> exception and document it? The IdentifierUtils don't declare any exceptions
> and they were meant as use-and-forget tool ...
>
One thing that I would like for us to consider is to omit the uuid
generators from 1.0. There is a lot to do to get that package into
releaseable state, including spec compliance review, and I am a little
concerned with our ability to support it once released, as the
original developer is no longer working on this code. I would be +1
to cleaning up the rest of the package, promoting to proper and
releasing a simple 1.0 without the uuid generators. I think the
package is quite useful without the uuid functions and if there is
sufficient community interest and support we can add the uuid stuff
into a 1.1.
Phil
---------------------------------------------------------------------
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
