On 07/15/2013 11:04 AM, Aleksey Shipilev wrote:
Hi, Peter,
Not a reviewer, and have barely any time to have the careful review, so
just a few nitpicks below:
On 07/09/2013 12:54 AM, Peter Levart wrote:
http://cr.openjdk.java.net/~plevart/jdk8-tl/AnnotationType/webrev.05/
AnnotationParser.java:
- Do you want make contains() generic?
static <T> boolean contains(T[] array, T element);
Hi Aleksey,
I was guided by the Collection.contains() signature:
Collection<T> {
boolean contains(Object o);
Now I could do this:
static <T> boolean contains(T[] array, Object element);
But 'T' then looses it's purpose. I think the method is good as is. It's
type-safe for any input.
- Also, you might probably put the null-check in contains(), and
simplify the usage
I know it's a matter of style mostly, but when I see the following code:
if (selectAnnotationClasses != null &&
!contains(selectAnnotationClasses, annotationClass)) {
I don't have to go an check what contains() method does. It's obvious.
If there was only the following:
if (!contains(selectAnnotationClasses, annotationClass)) {
I would immediately jump up with questions: What about when
selectAnnotationClasses is null? Will there be NPE, false or true
answer? Let's check what contains() does...
The usage might seem simplified, but it would be too simplified in my
opinion. Now if there were 2 such usages or more, it would be enough to
grant creating such method and naming it 'isNullOrContains()' or
similar, but for a single usage I think this strikes the right balance
between explicit and concise coding.
- parseAnnotation2 seems a bad name; it seems just overloading
parseAnnotation is good.
Even here I followed the prior art. Since public parseAnnotations() (and
now also package-private parseSelectAnnotations()) delegates to private
parseAnnotations2() to do most of the work, I inherited the style and
made package-private parseAnnotation() delegate to private
parseAnnotation2() to do the parsing. While I agree using '2' suffix (or
'0' as in j.l.Class and friends or '_' prefix somewhere else) could be
replaced with overloading the same name, I also think that using
distinct names makes code easier to follow mentally. But I don't like
'2' or '0' or '_' either. So what do you suggest? Maybe
privateParseAnnotation(s), or parseAnnotation(s)Impl? It makes sense to
overload names in public API but for internal private usage I think it's
best to use distinct names (if not for other things, because it's easier
to refer to a specific method in texts like messages on mailing list,
bug reports, stack-traces, etc...).
So should we change '2' to some prefix/suffix word then?
Regards, Peter
-Aleksey.
