Hi Benedikt,
> 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 had a look at your patch - and still wondering how you could have
attached it, since messages with attachments should be rejected by ASF
mail server - and, don't get me wrong or get it personally, I tink you
"abused" of the principle you follows.
There a a couple of excellent ideas, i.e. moving
checkTypesCompatible() in TypeUtils would be helpful to be reused in
Constructors, where same logic has been applied so there's real method
reusability.
OTOH, I didn't see a big advantage of splitting just ~65 lines of code
method in a multiple stack-method-calls, take a look at how deep it
looks now:
resolve
├── resolveDirectly
└── resolveMethodByParameterTypes // here you tores ALL methods/ratings
├── checkTypesCompatible
├── getAccessibleMethod
├── getAccessibleMethodFromInterfaceNest
├──
getAccessibleMethodFromInterfaceNest
└── getAccessibleMethodFromSuperclass
├── getTotalTransformationCost
└── getBestMatch
Doesn't this lasagna have too much layers? While splitting
resolveDirectly/resolveMethodByParameterTypes could be good, for the
few lines of code that the original method is composed by, there is a
little of overengineering.
Lasagna is good - I'm Italian and I like it! ;) - but don't forget
that too much carbohydrates can cause obesity as well ;)
> 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 honestly think that you are a very talented guy who'll quickly
become a Commons Committer, and I suppose I am older - even if a
little - than you so, even if not strictly related to BeanUtils2, try
to follow as much as possible a small suggestion from an older guy -
that could be generally applied, not being related just to technology:
reading great books from gurus is really good, *following dogmas* is
evil. Dogmas are for fanatic religious, you have a very well working
brain that doesn't need strict rules.
Best practices are one thing, universally applied principles are different.
Have a nice Sunday, all the best,
-Simo
http://people.apache.org/~simonetripodi/
http://simonetripodi.livejournal.com/
http://twitter.com/simonetripodi
http://www.99soft.org/
On Sat, Jan 28, 2012 at 3:29 PM, Benedikt Ritter
<b...@systemoutprintln.de> wrote:
> 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
>
>
>
>
> ---------------------------------------------------------------------
> 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
