Pitfalls in getting member variable values in Java

问题:

I have an abstract class as follows. I want to get all the values of member variables.

public abstract class PARAMS {
    public static final String NAME1 = "VAL1";
    public static final String NAME2 = "VAL2";
    public static final String NAME3 = "VAL3";
}

The values are retrieved using reflection as follows.

Field[] fields = PARAMS.class.getFields();
for (Field field : fields) {
    String name = field.getName() ;
    String value = (String) field.get(name);
}

This is the first time I am experimenting with reflection. Is this a correct way to achieve the goal? I would like to know what are the pitfalls in using reflection in this case.

回答1:

You code iterates over both static and private fields. So you should check that you iterate over static fields only.

for (Field field : PARAMS.class.getFields()) {
    if (Modifiered.isStatic(field.getModifiers())) continue;
    String name = field.getName() ;
    String value = (String) field.get(PARAMS.class);
}

NB: as Jon mentioned, for static field access the instance parameter is ignored. However, I prefer passing in the class instead of null since this is a better documentation of the indent.

It is however even better practice to annotate your fields with an annotation so that you only get those fields that you really want no other static fields added by other programmers (or even the Java language behind the scenes). If you do so, your code would look like

for (Field field : PARAMS.class.getFields()) {
    if (!field.isAnnotationsPresent(YourAnnotation.class)) continue;
    String name = field.getName() ;
    String value = (String) field.get(PARAMS.class);
}

回答2:

It's not quite correct - the argument to get should ideally be null for the sake of readability: the point of that argument is to give it a target for when you're retrieving instance fields.

So your code can be just:

Field[] fields = PARAMS.class.getFields();
for (Field field : fields) {
    String name = field.getName() ;
    String value = (String) field.get(null);
}

Now, this should work... but what are you going to do with these values? Is there any reason why you want to do this rather than creating an immutable Map<String, String> which is exposed directly?

Reflection is fine where it's necessary, but you haven't given enough information to determine whether it's actually necessary in this case.

回答3:

another problem, getFields return all accessible fields (static or not) of this class and all its superclasses. Not a problem for the specific code you posted, since the only superclass is Object which has no public field.
I would at least test if the field is declared in the correct class - getDeclaringClass() - and if it has the correct return type - getType().

Using an Annotation, as Adrian suggested, is best IMHO.