Greg, Replies below
Chris On 15 September 2010 18:38, Greg Brown <[email protected]> wrote: > 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. > > OK, will do. > + @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. > I was thinking it might be useful in its own right & the logic is different enough from the primitives coercion. If you'd rather have it inline, I can make the change. > > + } > > + 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. > > Will make the change. > > + > > + // 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. > > Just trying to reduce code repetition, but I know it is clunky. I should have removed it earlier actually, as I simplified the exception message so it can just as easily be created using a constant as you suggested. I blame delays between writing, testing and checking in for not catching it. :)
