commons-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Benedikt Ritter (JIRA)" <j...@apache.org>
Subject [jira] [Comment Edited] (SANDBOX-462) Refactoring of AccessibleObjectsRegistry
Date Mon, 17 Feb 2014 18:06:19 GMT

    [ https://issues.apache.org/jira/browse/SANDBOX-462?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13903410#comment-13903410
] 

Benedikt Ritter edited comment on SANDBOX-462 at 2/17/14 6:05 PM:
------------------------------------------------------------------

Hello André,

first of all: thanks for this contribution! I've reviewed it but I think we have to tidy it
up a bit before we can get it into trunk :-) Here are my findings:

>From the description I can see that you have at least four changes in mind. It is better
to keep the change sets of patches as small as possible. That makes it a lot easier to review
patches. So I'd suggest that you create one patch for every bullet point.
 
Your patch contains a lot of unrelated changes, mostly due to auto formatting of your IDE.
See for example this lines in your patch file:
{code}
-abstract class AccessibleObjectsRegistry<AO extends AccessibleObject & Member>
-{
+abstract class AccessibleObjectsRegistry<AO extends AccessibleObject & Member>
{
{code}
This change just places the curly brace at the end of the line. Please remove this unrelated
changes. BeanUtils uses the maven coding conventions. A fact that I'm not completely happy
with because no IDE has this as a standard configuration. But for now it is as it is and we
should stick with it until we have come to another decision on the ML.

I can see in your patch file that you've used your IDE to generate it. This is perfectly fine,
but I personally prefer the command line, since it gives you finer control over what happens.
Maybe you should give it a try?

As far as I can see you haven't yet filed an [ICLA|http://www.apache.org/licenses/#clas].
Please do so as soon as you find the time. If you're planning to contribute on a regular basis,
this is an mandatory requirement.

If you have any problems with getting the patch right, don't hesitate to ask here or on the
ML.

Regards,
Benedikt


was (Author: britter):
Hello André,

first of all: thanks for this contribution! I've reviewed it but I think we have to tidy it
up a bit before we can get it into trunk :-) Here are my findings:

>From the description I can see that you have at least four changes in mind. It is better
to keep the change sets of patches as small as possible. That makes it a lot easier to review
patches. So I'd suggest that you create one patch for every bullet point.
 
Your patch contains a lot of unrelated changes, mostly due to auto formatting of your IDE.
See for example this lines in your patch file:
{code}
-abstract class AccessibleObjectsRegistry<AO extends AccessibleObject & Member>
-{
+abstract class AccessibleObjectsRegistry<AO extends AccessibleObject & Member>
{
{code}
This change just places the curly brace at the end of the line. Please remove this unrelated
changes. BeanUtils uses the maven coding conventions. A fact that I'm not completely happy
with because no IDE has this as a standard configuration. But for now it is as it is and we
should stick with it until we have come to another decision on the ML.

I can see in your patch file that you've used your IDE to generate it. This is perfectly fine,
but I personally prefer the command line, since it gives you finer control over what happens.
Maybe you should give it a try?

As far as I can see you haven't yet filed an [ICLA|http://www.apache.org/licenses/]. Please
do so as soon as you find the time. If you're planning to contribute on a regular basis, this
is an mandatory requirement.

If you have any problems with getting the patch right, don't hesitate to ask here or on the
ML.

Regards,
Benedikt

> Refactoring of AccessibleObjectsRegistry
> ----------------------------------------
>
>                 Key: SANDBOX-462
>                 URL: https://issues.apache.org/jira/browse/SANDBOX-462
>             Project: Commons Sandbox
>          Issue Type: Improvement
>          Components: BeanUtils2
>            Reporter: Andre Diermann
>            Priority: Minor
>         Attachments: Commons-BeanUtils2-462.patch
>
>
> Summary:
> The AccessibleObjectsRegistry class provides two get methods, while one is a convenient
method for the other.
> Both methods take one conditional parameter, boolean exact, and the actual get method
is very long, which makes it somehow complex to understand.
> Suggestion:
> What could be improved IMHO:
> - Instead of using conditional methods, like get(boolean doSomethingSpecialIfTrue, ...),
it is more convenient to provide dedicated methods like getSomething() and getAnotherThing().
> - In this regard the difference between an exact or, let's call it, matching descriptor
should be expressed through inheritance rather than object allocation (= expressing it by
a field boolean exact).
> - The very long get method should be refined
> - Another very minor issue is the naming of the paramTypes field within the inner AccessibleObjectDescriptor
class, which I would suggest to rename to parameterTypes to fit the naming of the other occurrences.




--
This message was sent by Atlassian JIRA
(v6.1.5#6160)

Mime
View raw message