commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Stephen Colebourne <scolebou...@btopenworld.com>
Subject Re: [id] Review before 1.0 (Summary)
Date Sat, 14 Jan 2006 21:18:54 GMT
As per the general thrust of Phil's mail, we should remove everything 
thats complex and not working and get a simple 1.0 out there. We can 
then grow from that point as and when the desire arises.

Stephen


Phil Steitz wrote:
> On 1/14/06, Jörg Schaible <joerg.schaible@gmx.de> 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: commons-dev-unsubscribe@jakarta.apache.org
> For additional commands, e-mail: commons-dev-help@jakarta.apache.org
> 
> 

---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org


Mime
View raw message