Review request for type annotation reflection test

[Sergey Grinev]

Also few minor comments:

1. Helper#getAnno()

   StringBuffer result = new StringBuffer("");

using StringBuilder is recommended for thread-safe methods

2. List Helper.getMultipleAT()

this method uses List where List<String> can be specified

3. Helper.compareAnnoWithDiffSeq()

this can be done much shorted and without copying array to list:

         String[] split1 = anno1.trim().split("\\s+");
         String[] split2 = anno2.trim().split("\\s+");
         Arrays.sort(split1);
         Arrays.sort(split2);
         return Arrays.equals(split1, split2);

4. Helper.compileCode():697-704 can be simplified using jdk8 streams:

ok = diagnostics.getDiagnostics().stream().anyMatch(d -> d.getKind() == Diagnostic.Kind.ERROR);

5. Same for Helper.isPkgInfoPresent:

return files.stream().map(JavaFileObject::getName).anyMatch(name -> name.contains("package-info") || name.contains("PkgAnno"));

6. AnnotatedArrayTypeGetAnnotatedGenericComponentTypeTest:175

You are calling Helper.getArrAT(at) then split it by ";" and work with 
result.
while Helper.getArrAT() takes a list and convert it into string:
return getAnno(annotations) + ";" + ret;

It may be more efficient to work directly with lists.

7. DeclarationGenerator interface is never used as interface

8. Helper.Declaration.replaceAnnoStr should be static

9. Helper.Declaration.replaceAnnoStr/getFirstAnno return result as 
String[] array, which is considered to be a bad practice (you are using 
variable length structure to store exactly 2 values). It would be 
cleaner to introduce intermediate structure of 2 string to store return 
value.

-- Sergey

On 23-Aug-13 14:36, Sergey Grinev wrote:
> Hi, all.
>
> Sorry if this review covers same issue I reported on reviewboard one, 
> but it seems few important point were not fixed:
>
> 1.  public Map<Helper.Declaration, Object> lhm = new LinkedHashMap<>();
> and
>     public synchronized static String genDeclaration(
>             Map<Helper.Declaration, Object> input, Declaration... 
> srcType)
>
> - fields used as a method-local temporary variables are error-prone 
> and confusing. Users/readers of that code are not aware when this 
> field can be used or when this field can be cleared, also using such 
> field is not thread-safe. Imagine someone added one more ENUM value to 
> TestCase, used lhm and didn't call lhm.clear() -- things will break 
> and very different place this person didn't even touch.
>
> - Map claims it stores Objects but actually they are only String and 
> String[]. Thus your getDeclaration() has invalid signature and is 
> error-prone. Also you calls instanceof several times to resolve this 
> ambiguity which is considered to be an anti-pattern as well.
>
> You can solve these problems by introducing separate entity 
> responsible for declaration generation:
>
> public class DeclarationGenerator {
>
>     private final Map<Helper.Declaration, String[]> lhm = new 
> LinkedHashMap<>();
>         public void add(Helper.Declaration key, String... value) {
>         lhm.put(key, value);
>     }
>     public String genDeclaration(Declaration... srcType) {
>         //...
>         for (Declaration st : Declaration.values()) {
>                 if (input.containsKey(st))
>                         result.append(st.getSrc(input.get(st)));
>         }
>         //...
>     }
> }
>
> with this helper class instead of
>
>                 lhm.clear();
>                 lhm.put(Helper.Declaration.IMPORT, null);
>                 lhm.put(Helper.Declaration.CLASS, str[0]);
>                 testCode += Helper.genDeclaration(lhm);
>
> you can write
>
>                 DeclarationGenerator dg = new DeclarationGenerator(); 
> // not to reuse, create new one for each generation
>                 dg.put(Helper.Declaration.IMPORT, null);
>                 dg.put(Helper.Declaration.CLASS, str[0]);
>                 testCode += dg.genDeclaration();
>
> having clean design without field used as temporary variable and type 
> ambiguity.
>
>
> 2. several TestCase enum entries, especially TESTBASECLSSTART and 
> TESTBASECLS are copypasted in several places.
>
> 3. public static Map<String, String> testInput = new LinkedHashMap<>();
>
> why this field in static?
>
>
> -- Sergey
>
> On 22-Aug-13 22:43, Alex Buckley wrote:
>> I have no further comments. Thanks for all your work to test this new 
>> and expansive API.
>>
>> Alex
>>
>> On 8/20/2013 1:56 AM, Charlie Wang wrote:
>>> Hi Alex,
>>>    I used testInput.size() instead of static int clsIdx as suffix this
>>> time. And other update according to your comments.
>>>
>>> Here's the link. Please review again.
>>> http://cr.openjdk.java.net/~pzhang/Charlie/TypeAnnotation1/webrev/
>>>
>>>
>>> - Charlie
>>>
>>> On 2013/8/9 1:05, Alex Buckley wrote:
>>>> - AnnotationTest.i/clsIdx: it wasn't the name of the variable I was
>>>> objecting to. It was the fact that you're relying on a mutable public
>>>> static variable at all.
>>>>
>>>> - GenTestCode should take Class<? extends TestCaseGenerator>, so you
>>>> don't need to cast to TestCaseGenerator in GenTestCode. (Same comment
>>>> for AnnotationTest.compileCode).
>>>>
>>>> - AnnotationTest.testInput is a Map<Object,String> but I'm sure you
>>>> can do better than Object for keys.'
>>>>
>>>> - ExecutableGetAnnotatedParameterTypesTest.testInput hides
>>>> AnnotationTest.testInput, but swaps the order of the type arguments!
>>>> Map<String,Object> is either wrong, or should not use Object.
>>>>
>>>> Alex
>