commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jörg Schaible <joerg.schai...@gmx.de>
Subject [id] Review before 1.0 (Summary)
Date Sat, 14 Jan 2006 13:45:51 GMT
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.

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.

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();
}

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.

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 ...


Please comment on the topics.

- Jörg


---------------------------------------------------------------------
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