Review request for type annotation reflection test

[Sergey Grinev]

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