commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benedikt Ritter <b...@systemoutprintln.de>
Subject Re: [SANDBOX][BeanUtils2] Thoughts about AccessibleObjectsRegistry
Date Sat, 28 Jan 2012 14:28:37 GMT
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-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org


Mime
View raw message