Review request for type annotation reflection test

[Charlie Wang]

Hi Sergey,
   I've updated my code according to your comments. Please take a look 
again.
http://cr.openjdk.java.net/~pzhang/Charlie/TypeAnnotation1/webrev/

P.S. When updating Helper.isPkgInfoPresent with: return 
files.stream().map(JavaFileObject::getName).anyMatch(name -> 
name.contains("package-info") || name.contains("PkgAnno"));
There is a compile error. So the code stays the original way. Due to the 
urgency of the test, I will file a RFE bug to track it and update the 
piece of code when time allows.



- Charlie


On 2013/8/27 2:27, Sergey Grinev wrote:
> Hi Charlie,
>
> please, take a look at few inline comments.
>
> -- Sergey
>
> On 26-Aug-13 11:45, Charlie Wang wrote:
>> 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.
>
> You may want to consider using separate enum for all your cases (or 
> one per package).
>
>>
>>>
>>> 3. public static Map<String, String> testInput = new LinkedHashMap<>();
>>>
>>> why this field in static?
>>>
>> Made it static for enum methods to access directly.
>
> Oh I missed usages of this field, sorry. Current approach isn't the 
> best from OOD point of view. You have static field cleaned in 
> constructor and field being reused from the code outside of the 
> AnnotationTest. This leads to uncontrollable logic flow and 
> impossibility of parallel invocation.
>
> The convenient approach would be
> 1) either having this field non-static and provide instance of 
> AnnotationTest or testInput as parameter to each genTestCase() call.
> 2) or using anonymous classes (or even lambdas) instead of enum, which 
> allows convenient access to "this" without statics:
>
>     private final static TestCaseGenerator TESTBASECLSSTART1 = (String 
> str) -> {
>         testInput.put(null, null); // testInput not static
>
>         // other code
>
>         return "";
>     };
>
> P.S.: AnnotationTest.GenTestCode() method should be *g*enTestCode() by 
> common Java code guidelines.
>
>>
>>
>> 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.
> Agree.
>>
>>>
>>> 8. Helper.Declaration.replaceAnnoStr should be static
>> Isn't it static already.
>
>     http://cr.openjdk.java.net/~pzhang/Charlie/TypeAnnotation1/webrev/raw_files/new/test/type-annotations/Helper.java
>     <http://cr.openjdk.java.net/%7Epzhang/Charlie/TypeAnnotation1/webrev/raw_files/new/test/type-annotations/Helper.java>
>
>     private String[] replaceAnnoStr(String ss, String temp,
>
>
> Seems no.
>>
>>>
>>> 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.
>
> But that's the point. It would be class with fields with meaningful 
> names instead of [0] and [1]. Anyone who will be 
> supporting/fixing/using your code in future would be grateful for 
> that. And you will avoid using array to store fixed length data which 
> considered to be a bad practice.
>
> Thanks,
>
> Sergey.
>
>>
>>
>> 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
>>>
>>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/type-annotations-dev/attachments/20130906/47e25645/attachment.html