commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From sebb <seb...@gmail.com>
Subject Re: [digester] help on graduating at-digester sandbox to current implementation
Date Mon, 25 Jan 2010 02:59:32 GMT
On 24/01/2010, Simone Tripodi <simone.tripodi@gmail.com> wrote:
> Hi all guys,
>  I'm writing you because I really need your help on graduating my effort
>  spent on sandbox about the digester: I finally terminated porting my old
>  stuff[1] on the sandbox and implemented some test cases that show how to use
>  annotations on pojos.
>  I need your help on:
>  - reviewing the design;
>  - make the code more compliant to commons-digester;
>  - increasing test cases
>  - suggest me what I have to do to propose the sandbox merged in /trunk
>  - help me on writing the documentation, I'm not a native English speaker :(
>  :(
>
>  Is anyone available to help me? I'll start writing an architectural document
>  to provide you better informations, and apologize in advance for my poor way
>  to explain concepts in English :P
>
>  In the meanwhile, if you have spare time to have a look at the code you can
>  check it out from svn sandbox[2]

I cannot comment on the design or behaviour, as I'm not familiar with Digester.

However, I can comment on the coding style.

There are a few missing @Override markers.

@SuppressWarnings("unchecked") should be used as close as possible to
the statement that causes the warning, and there should be a comment
as to why the warning can be ignored.

For example, in DefaultLoaderHandler.handle(), the annotation should
be used on the first statement of the try{} block. Similarly for
.MethodHandler.doHandle(), etc.

There are a few private methods where the Javadoc is incomplete, e.g.
MethodHandler.doHandle(). Either drop the Javadoc, or have something
useful there.

The Javadoc for MethodArgument() references a non-existent parameter;
there are a few other Javadoc problems, e.g. the @see references.

It would be useful to document which classes are intended to be
thread-safe, and which are definitely not intended to be thread-safe .

Could the AnnotationRuleProvider.init() method be dropped in favour of
providing the parameters in the constructors? That would allow the
fields to be made final, which would improve thread-safety.

In the MethodArgument class, two of the methods return a pointer to
the final Annotations array. This breaks containment, as it allows
external classes to modify the array. It would be safer to return a
copy of the array. This would also make the class immutable and
therefore thread-safe.

Findbugs warns that the field FromAnnotationsRuleSet.namespaceURI is
never written.
This must be a mistake.

Having said all that, I think the general style seems better than the
existing Digester code, which has a lot of problems, e.g. public
static fields that are not final! [Any such issues should be fixed in
trunk, not here].

By the way - did you need to make any changes to the existing Digester code?

>  Thanks in advance, all the best!!!
>  Simo
>
>  [1] http://code.google.com/p/digester-annotations/ - still on google code,
>  do I have to drop it?
>  [2] https://svn.apache.org/repos/asf/commons/sandbox/at-digester/trunk
>
>  http://people.apache.org/~simonetripodi/
>

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


Mime
View raw message