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] [Commented] (SANDBOX-472) Transformer Registry
Date Sat, 26 Jul 2014 10:30:38 GMT

    [ 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)

Mime
View raw message