[
https://issues.apache.org/jira/browse/SANDBOX-462?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13903410#comment-13903410
]
Benedikt Ritter commented on SANDBOX-462:
-----------------------------------------
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)
