Hi Chris,
Looks good - thanks for tackling this one. Comments below:
> Modified: pivot/trunk/core/src/org/apache/pivot/beans/BeanAdapter.java
> @@ -841,6 +844,8 @@ public class BeanAdapter implements Map<
> if (type.isAssignableFrom(value.getClass())) {
> // Value doesn't need coercion
> coercedValue = value;
> + } else if (type.isEnum() && value instanceof String) {
> + coercedValue = coerceEnum(value, type);
Since this is a coercion method, it might be more flexible to check for value
!= null and call toString() on it.
> + @SuppressWarnings("unchecked")
> + private static <T> T coerceEnum(Object value, Class<? extends T> type) {
Was there a reason you put this in a separate method? I imagined we'd just
inline it in the coerce method.
> + }
> + if (!(value instanceof String)) {
> + throw new IllegalArgumentException(String.format(
> + "Non-null String value must be supplied for enum coercion.
> Value=%s",
> + (value == null ? null : value.getClass().getName())));
> + }
Same comment here as above - it would probably be more flexible to call
toString() rather than cast to String. Also, null is a valid enum value, so we
should allow it.
> +
> + // Find and invoke the valueOf(String) method, with an upper case
> + // version of the supplied String
> + T coercedValue = null;
> + try {
> + Method valueOfMethod = type.getMethod(ENUM_VALUE_OF_METHOD_NAME,
> String.class);
> + if (valueOfMethod != null) {
> + String valueString = ((String)
> value).toUpperCase(Locale.ENGLISH);
> + coercedValue = (T) valueOfMethod.invoke(null,
> valueString);
> + }
> + }
> + // Nothing to be gained by handling the getMethod() & invoke()
> + // Exceptions separately
> + catch (IllegalAccessException e) {
> + throwCoerceEnumException(value, type, e);
In general, we don't define "thrower" methods. You could instead define a
constant for your exception message and re-use that in each catch block. Later,
when we (hopefully) get multi-catch in JDK 7, we can consolidate blocks like
this.
Greg
