"wolfc" wrote:
It should then check whether the declaring class is assignable from the target class.

 protected static void checkMember(Object target, Member member, String memberType) throws Throwable
 {
 if (member == null)
 throw new IllegalArgumentException("Null " + memberType);

 if (target != null)
 {
 Class<?> declaringClass = member.getDeclaringClass();
 if (declaringClass.isInstance(target) == false)
 throw new IllegalArgumentException("Target (" + Strings.defaultToString(target) + ") doesn't match " + memberType + " 's (" + member + ") declaring class.");
 }

 }

That's not correct. It's now doing the check twice.
Once when you do it and once inside the JDK reflection api.

That's horribly inefficient, since these reflection calls are used everywhere.

The whole point of ReflectionUtils is give more reasonable error messages
after reflection calls fail, because the JDK is usually hopeless.

e.g. you'll get something like

java.lang.IllegalArgumentException: wrong type java.lang.String

 public static Throwable handleErrors(String context, Object target,
+ Class<?> clazz,
+ Object instance,
 Class<?>[] parameters, Object[] arguments, Throwable t) throws Throwable
 {
 if (t instanceof IllegalArgumentException)
 {
 if (target == null)
 throw new IllegalArgumentException("Null target for " + context);
+ if (clazz != null && instance != null && clazz.isAssignableFrom(instance) == false)
+ throw new IllegalArgumentException("Wrong target. " + instance.getClass().getName() + " is not a " + clazz.getName());
 ArrayList<String> expected = new ArrayList<String>();
 if (parameters != null)
 {
 for (int i = 0; i < parameters.length; ++i)
 expected.add(parameters.getName());
 }
 ArrayList<String> actual = new ArrayList<String>();
 if (arguments != null)
 {
 for (int i = 0; i < arguments.length; ++i)
 {
 if (arguments == null)
 actual.add(null);
 else
 actual.add(arguments.getClass().getName());
 }
 }
 throw new IllegalArgumentException("Wrong arguments. " + context + " for target " + target + " expected=" + expected + " actual=" + actual);
 }
 else if (t instanceof InvocationTargetException)
 {
 throw ((InvocationTargetException) t).getTargetException();
 }
 throw t;
 }

 public static Object invoke(Method method, Object target, Object[] arguments) throws Throwable
 {
 if (method == null)
 throw new IllegalArgumentException("Null method");
 try
 {
 return method.invoke(target, arguments);
 }
 catch (Throwable t)
 {
 throw handleErrors(method.getName(), Strings.defaultToString(target),
+ method.getDeclaringClass(), target,
 method.getParameterTypes(), arguments, t);
 }
 }

With the current methods on ReflectUtils this only comes in play with method and field invocation, so I only added that code there - and not to handleError method.

The point is that it shouldn't do anything at all unless there is an error.

You effectively have:

public void doSomething(X parameter)
{
 check(parameter);
 doIt(parameter);
}

private void doIt(parameter)
{
 check(parameter);
 ...
}

public void doSomething(X parameter)
{
 try
 {
 doIt(parameter);
 }
 catch (Exception e)
 {
 throw fixErrorMessage(e);
 }
}

"adrian@jboss.org" wrote:
The point is that it shouldn't do anything at all unless there is an error.