[
https://issues.apache.org/jira/browse/SANDBOX-472?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14075327#comment-14075327
]
Benedikt Ritter commented on SANDBOX-472:
-----------------------------------------
Hello Yogesh,
I've applied your changes from SANDBOX-473 and now I've reviewed your patch.
Here are my findings:
* Can you please create only one patch that contains all changes? If you're
using eclipse you just have to select the project root and create the patch
from there. If you're using the command line, just type {{svn diff >
sandbox-472.patch}}
* Please make sure you produce valid JavaDoc. Since Java 8, the JavaDoc tool is
very strict. For example the class JavaDoc would be invalid since you're not
closing the second {{<p>}} tag.
* Don't add empty JavaDoc (as for the CLASS_TYTPE_NAME_PREFIX and the
transformers map). It just clutters the code. (The same is true for out
generated Eclipse JavaDocs in the TestStringIntegerTransformerImpl)
* You should better validate that sourceType and destinationType are not null
in the TransformerMapKey's constructor. This way you don't have to do the
validation in every method later on.
* Don't use different terms for the same thing. Your implementing a
TransformerRegistry but the JavaDoc for {{register(Transformer<?, ?>)}} talks
about "the container". Better use the term registry here.
* Don't add a dash after the parameter name in {{@param}} JavaDoc tags
* Sentences should start with an upper case letter (the JavaDoc of the
{{deregister}} methods doesn't)
* The getClass method doesn't look useful to me. You're using it in
{{deregister(Transformer<?, ?>)}} to get the Class object of a type (which is
unsafe btw, since an instance of Type is not necessary an instance of Class),
then you're passing the types into a TransformerMapKey which itself works with
Types. So there is no need to get the classes.
* Why is only the targetType checked for null in {{lookup(Class<?>, Class<?>)}}?
* Lookup should return null if the transformer can not be found.
* Please remove the "Impl" postfix from the test transformers. Using the "Impl"
postfix is an anti pattern, IMHO...
Regards,
Benedikt
> Transformer Registry
> --------------------
>
> Key: SANDBOX-472
> URL: https://issues.apache.org/jira/browse/SANDBOX-472
> Project: Commons Sandbox
> Issue Type: Task
> Components: BeanUtils2
> Affects Versions: Nightly Builds
> Reporter: Yogesh Rao
> Fix For: Nightly Builds
>
> Attachments: TestStringFloatTransformerImpl.java.txt,
> TestStringIntegerTransformerImpl.java.txt, TransformerRegistry.java.txt,
> TransformerRegistryTestCase.java.txt
>
>
> Hi,
> This is my first development JIRA for BU2 so apologies in case i m missing
> out on basics. Beanutils1 has a functionality wherein all the converters are
> registered and are called when conversion in value is required. This
> functionality is missing for BU2 project. I also saw that BeanUtils1 uses
> WeakFastHashMap for this, which seems like is having issues across
> architectures and did see few JIRA's on this. Wanted inputs if having a
> synchronized instance of WeakHashMap wrapped in the TransformerRegsitry class
> and providing methods to register, deregister, restoreDefault, lookup be
> desired?
--
This message was sent by Atlassian JIRA
(v6.2#6252)
