You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@commons.apache.org by Benedikt Ritter <be...@systemoutprintln.de> on 2012/01/27 17:56:07 UTC
[SANDBOX][BeanUtils2] Thoughts about AccessibleObjectsRegistry
Hi,
now, that I have made myself more familiar with BeanUtils2, there are a
few things worth discussing regarding AccessibleObjectsRegistry.
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.
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.
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).
all the best
Benedikt
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [SANDBOX][BeanUtils2] Thoughts about AccessibleObjectsRegistry
Posted by Benedikt Ritter <be...@systemoutprintln.de>.
Am 30.01.2012 12:46, schrieb Simone Tripodi:
> Hi Benedikt,
>
>>
>> The patch for TypeUtils has been created yesterday based on r1237257. Please
>> have a look at SANDBOX-369.
>>
>
> yes saw it, I am subscribed to Jira notifications ML, if I didn't have
> already look at it it is because, unfortunately, my paid job doesn't
> involve BeanUtils ATM - so I have to postpone it to my spare time. It
> could be later, like it could be next month, or next year, or next
> life ;)
>
> -Simo
>
Oh, I'm sorry! I saw your commit of AccessibleObjectsRegistry yesterday,
and draw the conclusion, that you haven't seen the issue. Didn't mean to
put you under pressure ;-)
Have a nice and productive working day!
All the best,
Benedikt
>
> 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
Re: [SANDBOX][BeanUtils2] Thoughts about AccessibleObjectsRegistry
Posted by Simone Tripodi <si...@apache.org>.
Hi Benedikt,
>
> The patch for TypeUtils has been created yesterday based on r1237257. Please
> have a look at SANDBOX-369.
>
yes saw it, I am subscribed to Jira notifications ML, if I didn't have
already look at it it is because, unfortunately, my paid job doesn't
involve BeanUtils ATM - so I have to postpone it to my spare time. It
could be later, like it could be next month, or next year, or next
life ;)
-Simo
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
Re: [SANDBOX][BeanUtils2] Thoughts about AccessibleObjectsRegistry
Posted by Benedikt Ritter <be...@systemoutprintln.de>.
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
>> <be...@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
Re: [SANDBOX][BeanUtils2] Thoughts about AccessibleObjectsRegistry
Posted by Benedikt Ritter <be...@systemoutprintln.de>.
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.
>
> 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
> <be...@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
Re: [SANDBOX][BeanUtils2] Thoughts about AccessibleObjectsRegistry
Posted by Simone Tripodi <si...@apache.org>.
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
<be...@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
Re: [SANDBOX][BeanUtils2] Thoughts about AccessibleObjectsRegistry
Posted by Benedikt Ritter <be...@systemoutprintln.de>.
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
Re: [SANDBOX][BeanUtils2] Thoughts about AccessibleObjectsRegistry
Posted by Benedikt Ritter <be...@systemoutprintln.de>.
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
Re: [SANDBOX][BeanUtils2] Thoughts about AccessibleObjectsRegistry
Posted by Simone Tripodi <si...@apache.org>.
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-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org