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:
input: methodName, parameterTypes
output: c
get the direct method
return if found
else
for each method with given name
if parameterTypes.length != method argument type length
skip
for each method argument type
check if input argument type matches with current argument type
if method matches
bestMatchMethod <- calculateCost( method ) // this is
where the complex has been already split
return bestMatchMethod
can you describe please how you would intend splitting the method?
>
> 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...
>
> 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... :)
looking forward to read more from you, have a nice weekend, alles gute!
-Simo
[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
