On 25/01/2010, sebb <seb...@gmail.com> wrote:
> On 24/01/2010, Simone Tripodi <simone.trip...@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?
>
One other comment:
JUnit tests don't generally need main() methods or suite()s.
It's far too easy to accidentally omit test cases from a suite(),
especially when adding new tests.
> > 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-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org
