WIP:Added Internationalization module #228
Conversation
vapadwal
changed the title
There was a problem hiding this comment.
Thanks for providing this PR. However, I think here we more or less took over an existing legacy feature without properly questioning if that feature makes sense at all this way.
To what I understood now, this module seems to add the ability to provide a service that exposes the Java i18n resource-bundles to JSON. My assumption is that this might be used to reuse them on the client side. If we look at the way devon4ng resp. Angular is dealing with i18n I would put this entire module in question.
Further it is lacking our general code-standards:
- remove
@authortags - add
@sincetags - remove pointless
@interitDoc - add proper JavaDoc where missing
- Autoformat the code to avoid pointless blank lines
- I started the review and added some first review comments. However, I would question if we shoud do a full stop and actually dump this. I assume that nobody will ever use this code. If I am wrong please let me know and tell me your use-case.
| * @return Json String | ||
| * @throws Exception | ||
| */ | ||
| String getResourceObject(String locale, String filter) throws Exception; |
There was a problem hiding this comment.
Anti-Pattern: Do not declare API that throws Exception. We also have the best-practice in devon4j not to use checked exceptions:
https://github.com/devonfw/devon4j/blob/develop/documentation/guide-exceptions.asciidoc#exception-principles
| * Gets the JSON string for specified locale and filter | ||
| * | ||
| * @param locale locale | ||
| * @param filter |
There was a problem hiding this comment.
what is a filter here?
This JavaDoc is not helpful.
| * @return | ||
| * @throws Exception | ||
| */ | ||
| String getResourceAsJson(String locale, String filter) throws Exception; |
There was a problem hiding this comment.
So what is the difference between I18n and LocaleResourceFactory except the name of the method?
I do not get this and it seems kind of odd to me.
| throw de; | ||
| } catch (Exception e) { | ||
| LOGGER.error("Exception in getResourcesAsJSONStringUsingMMM ", e); | ||
| throw e; |
There was a problem hiding this comment.
| public String getResourceAsJson(String locale, String filter) throws Exception { | ||
|
|
||
| String strJSON = null; | ||
| HashMap<String, String> resourceMap = new HashMap<>(); |
There was a problem hiding this comment.
| if (locale == null || locale.isEmpty() || !LocaleUtils.availableLocaleSet().contains(objLocale)) { | ||
| throw new UnknownLocaleException(I18nConstants.INVALID_LOCALE); | ||
| } else { | ||
| resourceMap = (HashMap<String, String>) I18nUtils.getResourcesGeneratedFromMMMAsMap(objLocale); |
There was a problem hiding this comment.
Why cast at all if the method is implemented and defined by us?
| * @throws Exception thrown by getResourcesGeneratedFromMMMAsMap | ||
| */ | ||
|
|
||
| public static Map<String, String> getResourcesGeneratedFromMMMAsMap(Locale locale) throws Exception { |
There was a problem hiding this comment.
Avoid CAPITALIZED segments in names but use camlCaseAlsoForAbbreviationsLikeMmm.
https://github.com/devonfw/devon4j/blob/develop/documentation/coding-conventions.asciidoc#naming
| String subKey = filter.substring(0, indexOfDot); | ||
| JsonObject jsonObject = (JsonObject) objJSON.get(subKey); | ||
| count++; | ||
| strJSONKey = strJSONKey + "{\"" + subKey + "\":"; |
There was a problem hiding this comment.
It also seems an anti-pattern to me to generate JSON via "string-fiddling".
Is maybe gson the wrong choice here? I have not used it as there is a JEE standard JSON-P for it and we comitted to use JEE standards where suitable instead of using proprietary solutions:
https://github.com/devonfw/devon4j/blob/develop/documentation/guide-jee.asciidoc#jee
| /** | ||
| * Opening bracket | ||
| */ | ||
| public static final String OPENING_BRACE = "{"; |
There was a problem hiding this comment.
IMHO such stuff does not belong here:
- it is an implementation pointlessly exposed to the public
- it has nothing to do with I18n but is due to the fact that we decided to use JSON serialization here.
vapadwal
changed the title
|
I agree to @hohwille to dump and close this PR. Does not look like someone is working on it anyway |
No description provided.