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 Mon, 30 Jan 2012 10:47:50 GMT
Am 29.01.2012 12:37, schrieb Benedikt Ritter:
> Hi Simo,
>
> thank you so much for your feedback! Have a look at my comments inline...
>
> Am 29.01.2012 11:30, schrieb Simone Tripodi:
>> 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.
>
> No worries, I'm open for criticism. If I would be offended by someone
> taking his time, looking through my code, giving me suggestion for
> improvement, I would be a complete idiot.
>
>> 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.
>
> I'm happy to hear, that you liked that. I'll create an issue for that
> and write a patch.
>

The patch for TypeUtils has been created yesterday based on r1237257. 
Please have a look at SANDBOX-369.

>>
>> 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 ;)
>
> Splitting up methods isn't always about reusing logic. Often it is a
> matter of readability. It honestly took me nearly half an hour to
> understand what resolve was doing, before I created the patch. Methods
> should do exactly one thing and they should do it well. Looking back at
> the old version of resolve(), there was to much going on and to many
> local variable were used.
> Especially the computing of the best match was very obscure. The code
> comments helped me to understand, what was going on. Looking at my
> patch, I think that no comments are needed anymore, because now it is
> more self-explanatory.
> If you do not like the patch, I would recommend, that we should at least
> use a map to store matches, instead of managing 2 local variables.
>
>>
>>> 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,
>
> I'm very happy to hear that from you. Thank you!
>
> and I suppose I am older - even if a
>> little - than you so,
>
> How about me giving you some info about me, so that you know who you're
> dealing with. Here we go: I'm 25, living in a medium size town in the
> rhine area. I studied Information Systems Research at Duisburg-Essen
> university and am about to write my master thesis about modeling and
> managing of business process variants. I work at a BPM company in Bochum
> as a junior software engineer. The product I'm working on is a client
> server application for collaborative business process modeling. The
> client is build up from eclipse RCP, while the server is a Spring web
> application. And if you want to have a face to the name, have a look at
> http://www.wi-inf.uni-duisburg-essen.de/FGFrank/index.php?lang=de&&groupId=1&&contentType=FormerStudentGroupMember&&persId=1193
> (although that picture is about 2 years old...)
>
> 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.
>
> thanks again, I take that as a compliment ;-)
>
>>
>> Best practices are one thing, universally applied principles are
>> different.
>>
>> Have a nice Sunday, all the best,
>> -Simo
>
> You didn't share your thoughts about more test cases for invokeMethod().
> Shall we extend TestBean or just use java base classes?
>
> have a nice Sunday as well, a pesto!
> Benedikt
>
>>
>> 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
>> <bene@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-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
>>>
>>>
>>>
>>>
>>> ---------------------------------------------------------------------
>>> 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
>
>
> ---------------------------------------------------------------------
> 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