Whoopse, forgot to add the patch ;-) here it is...
Am 28.01.2012 15:28, schrieb Benedikt Ritter:
Hi Simo,
thanks for your answer! have a look at my inline comments:
Am 28.01.2012 08:26, schrieb Simone Tripodi:
Hi Benedikt,
thanks for investing your spare time on contributing on BeanUtils2!
Please read my inline comments:
Complex methods: I know, that BeanUtils2 is just an experiment, and
that the
code was written to get it working fast for this purpose. But some
methods
are really hard to read. Take for example resolve() on MethodsRegistry.
There are at least two things happening: 1. try to find a direct
match, 2.
if no direct match can be found, start a some how more complex search. I
think this could be separated into several methods. If you agree, I can
create an issue and write a patch.
what is the advantage of splitting that method? the algorithm looks
clear:
[...]
can you describe please how you would intend splitting the method?
I think the advantage is, that we need less local variables and that we
can read through the method(s) from an abstract point, going deeper and
deeper. Every method should only be concerned with, well one concern ;)
and that concern should be doing thinks on only one level of
abstraction. I am heavily influenced by Robert C. Martin's Clean Code
and I try to stick to his dogma, because I believe, that following his
style of writing software will lead to higher quality and better
readability.
I have attached a patch for AccessibleObjectRegistry where I tried to
split the method up, to make it more readable. Please let me know, what
you think about that. I think you said, that you are not that dogmatic,
the other day, so if you don't like it, just let me know.
If you like it and want to apply it, you should decide if you want to
move checkTypesCompatible() to TypeUtils (checkNotNull and
checkNoneIsNull will have to be added in that case).
Also, we have to make sure, that the behavior is the same as before. As
you can see in my code comments, with my implementation, the last best
match will be returned instead of the first (from your impl).
Another problem is, that we do not have enough test cases atm. We really
need some tests on methods, that accept more than one param in
StaticMethodsTestCase (and a test case for non static methods). We have
two options now:
1. extend TestBean with some dummy methods.
2. use native Java classes and jUnit classes for this purpose.
I'm +1 for option 2, what do you think?
Magic numbers: I really don't understand, how object transformation
costs
get calculated in MethodsRegistry.getObjectTransformationCosts. What
does
0.25f mean? And why do we add 1.5f? Are this values you came up with
from
the development of BeanUtils1? If so, this should be documented in
the code.
exactly, this has been merely copied from BeanUtils1 and it is not
clear to me as well - we should go reading the mail archive and/or
commit history to understand where they come from and comment,
extracting magic number as constants etc...
+1 I'll have a look at the resources you mentioned, when I have the time.
Terminology: I really don't like the name of the class. My opinion
is, that
the term "object" should never be overloaded. The problem is, that
AccessibleObjects refer to methods and constructors that are
accessible from
the caller (e.g. public for a caller outside the containing package
of the
bean class). Now, if you are new to BeanUtils2, your first thought
might be,
that those registries are holding objects, that are accessible
themselves.
What I'm trying to say is, that maybe the name should be changed to
something more convenient, for example
"AccessibleOperationsRegistry". Since
the general definition of a class is, that it defines the data and
possible
operations on that data for a set of similar objects, I think that name
would be very appropriate (AccessibleObjectDescriptor, etc should be
renamed
likewise).
-1 it is not a matter of taste, it comes from the nature of the
objects that the registry stores: both Method and Constructor
implement the AccessibleObject[1] interface, so a generic
implementation that stores AccessibleObject extensions, reused for
storing Method and Constructor, how else shall be called?
I don't like the therm `AccessibleObject` as well but I bet I would
require ages to get it changed in JVM spec... :)
Ah okay! That's a bit embarrassing, because it shows, that I do not know
the reflection API to well :)
But in this case I agree with you.
looking forward to read more from you, have a nice weekend, alles gute!
-Simo
you too, bye!
Benedikt
[1]
http://docs.oracle.com/javase/1.5.0/docs/api/java/lang/reflect/AccessibleObject.html
http://people.apache.org/~simonetripodi/
http://simonetripodi.livejournal.com/
http://twitter.com/simonetripodi
http://www.99soft.org/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org
Index:
src/main/java/org/apache/commons/beanutils2/AccessibleObjectsRegistry.java
===================================================================
--- src/main/java/org/apache/commons/beanutils2/AccessibleObjectsRegistry.java
(revision 1237051)
+++ src/main/java/org/apache/commons/beanutils2/AccessibleObjectsRegistry.java
(working copy)
@@ -21,7 +21,8 @@
import static java.lang.reflect.Modifier.isPublic;
import static java.security.AccessController.doPrivileged;
import static org.apache.commons.beanutils2.Assertions.checkArgument;
-import static org.apache.commons.beanutils2.TypeUtils.*;
+import static org.apache.commons.beanutils2.TypeUtils.getPrimitiveWrapper;
+import static org.apache.commons.beanutils2.TypeUtils.isAssignmentCompatible;
import java.lang.ref.Reference;
import java.lang.ref.WeakReference;
@@ -31,7 +32,9 @@
import java.lang.reflect.Modifier;
import java.security.PrivilegedAction;
import java.util.Arrays;
+import java.util.HashMap;
import java.util.Map;
+import java.util.Map.Entry;
import java.util.WeakHashMap;
import java.util.concurrent.locks.Lock;
import java.util.concurrent.locks.ReadWriteLock;
@@ -223,73 +226,125 @@
@Override
public Method resolve( boolean exact, Class<?> clazz, String
methodName, Class<?>...parameterTypes )
{
- // see if we can find the method directly
- // most of the time this works and it's much faster
+ // try to resolve method directly
+ Method resolvedMethod = resolveDirectly(clazz, methodName,
parameterTypes);
+ if ( resolvedMethod != null || exact )
+ {
+ return resolvedMethod;
+ }
+ else
+ {
+ // search through all methods
+ return resolveMethodByParameterTypes( clazz, methodName,
parameterTypes );
+ }
+
+ }
+
+ /**
+ * Tries to resolve a method directly by calling the Java reflect API.
+ *
+ * @param clazz the class to resolve the method from
+ * @param methodName the name of the method to be resolved
+ * @param parameterTypes the parameter types of the method to be
resolved
+ * @return the resolved method, or {@code null} if no method could be
resolved
+ */
+ private static Method resolveDirectly( Class<?> clazz, String
methodName, Class<?>... parameterTypes )
+ {
try
{
- Method method = clazz.getMethod(methodName, parameterTypes);
- if ( exact || method != null )
- {
- return method;
- }
+ return clazz.getMethod( methodName, parameterTypes );
}
catch ( NoSuchMethodException e )
{
/* SWALLOW */
- if ( exact )
- {
- return null;
- }
+ return null;
}
+ }
- // search through all methods
- int paramSize = parameterTypes.length;
- Method bestMatch = null;
- Method[] methods = clazz.getMethods();
- float bestMatchCost = Float.MAX_VALUE;
- float myCost = Float.MAX_VALUE;
- for ( int i = 0, size = methods.length; i < size; i++ )
+ /**
+ * Tries to resolve a method using by iterating through all methods of
a given class.
+ *
+ * @param clazz the class to resolve the method from
+ * @param methodName the name of the method to be resolved
+ * @param parameterTypes the parameter types of the method to be
resolved.
+ * @return the resolved method or {@code null} if no method could be
resolved.
+ */
+ private static Method resolveMethodByParameterTypes( Class<?> clazz,
String methodName,
+ Class<?>...
parameterTypes )
+ {
+ // this is still to confusing to understand what is happening on
first sight
+ Map<Float, Method> ratedMethods = new HashMap<Float, Method>();
+ for ( Method currentMethod : clazz.getMethods() )
{
- if ( methods[i].getName().equals( methodName ) )
+ if ( currentMethod.getName().equals( methodName ) )
{
- // compare parameters
- Class<?>[] methodsParams = methods[i].getParameterTypes();
- int methodParamSize = methodsParams.length;
- if ( methodParamSize == paramSize )
+ boolean match = checkTypesCompatible(
currentMethod.getParameterTypes(), parameterTypes );
+
+ if ( match )
{
- boolean match = true;
- for ( int n = 0; n < methodParamSize; n++ )
+ Method accessibleMethod = getAccessibleMethod( clazz,
currentMethod );
+ if ( accessibleMethod != null )
{
-
- if ( !isAssignmentCompatible( methodsParams[n],
parameterTypes[n] ) )
- {
- match = false;
- break;
- }
+ // this will override matches found earlier with
the same costs.
+ float costsForCurrent =
getTotalTransformationCost( parameterTypes,
accessibleMethod.getParameterTypes() );
+ ratedMethods.put(costsForCurrent, currentMethod);
}
-
- if ( match )
- {
- // get accessible version of method
- Method method = getAccessibleMethod( clazz,
methods[i] );
- if ( method != null )
- {
- myCost = getTotalTransformationCost(
parameterTypes, method.getParameterTypes() );
- if ( myCost < bestMatchCost )
- {
- bestMatch = method;
- bestMatchCost = myCost;
- }
- }
- }
}
}
}
- return bestMatch;
+ return getBestMatch(ratedMethods);
}
/**
+ * Checks if a set of parameters is compatible to another set. For
example you can check if all parameter types
+ * of one method are compatible to the parameter types of another
method.
+ *
+ * @param types the types check against otherTypes
+ * @param otherTypes the other types used for comparing
+ * @return true if all parameters are compatible
+ */
+ private static boolean checkTypesCompatible( Class<?>[] types,
Class<?>[] otherTypes )
+ {
+ if ( types.length != otherTypes.length )
+ {
+ return false;
+ }
+ for ( int n = 0; n < types.length; n++ )
+ {
+ if ( !isAssignmentCompatible( types[n], otherTypes[n] ) )
+ {
+ return false;
+ }
+ }
+ return true;
+ }
+
+ /**
+ * Gets the best match from a Map of rated methods, by comparing their
costs.
+ *
+ * @param ratedMethods a Map of rated methods
+ * @return the best match - the methods whose key has the smallest
value.
+ */
+ private static Method getBestMatch( Map<Float, Method> ratedMethods )
+ {
+ if ( ratedMethods.isEmpty() )
+ {
+ return null;
+ }
+
+ Entry<Float, Method> bestMatch =
ratedMethods.entrySet().iterator().next();
+ for ( Entry<Float, Method> entry : ratedMethods.entrySet() )
+ {
+ if ( entry.getKey() < bestMatch.getKey() )
+ {
+ bestMatch = entry;
+ }
+ }
+ return bestMatch.getValue();
+ }
+
+ /**
* <p>Return an accessible method (that is, one that can be invoked via
* reflection) that implements the specified Method. If no such method
* can be found, return <code>null</code>.</p>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org
