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:29:49 GMT
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-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