Review request for type annotation reflection test

[Charlie Wang]

Hi Alex,
   I think I will need to make update according to Sergey's comments 
before check in the code.

Sergey,
   Thanks for the comments. My reply inline.

On 2013/8/23 18: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.
>
I agree with you. I will update the code accordingly.

>
> 2. several TestCase enum entries, especially TESTBASECLSSTART and 
> TESTBASECLS are copypasted in several places.
I will remove the duplications.

>
> 3. public static Map<String, String> testInput = new LinkedHashMap<>();
>
> why this field in static?
>
Made it static for enum methods to access directly.


And as to the other comments from you:

> 1. Helper#getAnno()
>
>   StringBuffer result = new StringBuffer("");
>
> using StringBuilder is recommended for thread-safe methods
Agree.

>
> 2. List Helper.getMultipleAT()
>
> this method uses List where List<String> can be specified
Good check.

>
> 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);
OK.

>
> 4. Helper.compileCode():697-704 can be simplified using jdk8 streams:
>
> ok = diagnostics.getDiagnostics().stream().anyMatch(d -> d.getKind() 
> == Diagnostic.Kind.ERROR);
I didn't adopt it because I wasn't sure if I supposed to use this new 
jdk8 feature in this jdk8 test. Bus since Tristan also suggested it is 
doable, I will update my code this way.

>
> 5. Same for Helper.isPkgInfoPresent:
>
> return files.stream().map(JavaFileObject::getName).anyMatch(name -> 
> name.contains("package-info") || name.contains("PkgAnno"));
OK.

>
> 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.
The expected results (to be compared with test results) also come as 
strings because they are directly extracted from the test input, which 
is why I made it "getAnno(annotations) + ";" + ret" - so I don't have to 
split the result into lists. Either way, the conversion has to be made. 
The present way is more direct.

>
> 7. DeclarationGenerator interface is never used as interface
I don't think it hurts to put an interface there. At least it will help 
extension.

>
> 8. Helper.Declaration.replaceAnnoStr should be static
Isn't it static already.

>
> 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.
Then I will have to declare a new class for the two strings.


I will update my code according to most of your comments. Could you 
please reply to me asap if you think there's anything missing. Hopefully 
we can wrap things up within the week.



- Charlie


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