Vojtech Szocs has posted comments on this change.
Change subject: userportal,webadmin: update branding manager
......................................................................
Patch Set 5: (9 inline comments)
....................................................
File
backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/branding/BrandingManager.java
Line 72: /**
Line 73: * Constructor that takes a {@code File} object to configure the
brandingRootPath.
Line 74: * @param etcDir A {@code File} pointing to the branding root path.
Line 75: */
Line 76: private BrandingManager(final File etcDir) {
I assume this constructor exists only for unit testing purposes:
- why is this constructor private and not package-private, so that unit test
can use it directly?
- why do we need synchronized getInstance(File) method, given that unit test is
not exercising this class in multi-threaded way?
Line 77: brandingRootPath = new File(etcDir, BRANDING_PATH);
Line 78: }
Line 79:
Line 80: /**
Line 92: * Get an instance of the {@code BrandingManager} with the passed
in ETC_DIR.
Line 93: * @param etcDir a {@code File} pointing to the engine etc
directory.
Line 94: * @return A {@code BrandingManager}
Line 95: */
Line 96: public static synchronized BrandingManager getInstance(final File
etcDir) {
See my comment above, I think this method is just redundant, unit test can use
package-private constructor directly.
Line 97: if (instance == null) {
Line 98: instance = new BrandingManager(etcDir);
Line 99: }
Line 100: return instance;
Line 131: * Get the message associated with the passed in key.
Line 132: * @param key The key to get the message for. For instance
obrand.common.copy_right_notice.
Line 133: * @return The associated message in the default locale.
Line 134: */
Line 135: public static String getMessage(final String key) {
Why is this method static? Only singleton access methods like getInstance()
should be static.
Line 136: return getMessage(key, LocaleFilter.DEFAULT_LOCALE);
Line 137: }
Line 138:
Line 139: /**
Line 141: * @param key The key to get the message for. For instance
obrand.common.copy_right_notice.
Line 142: * @param locale The locale to use to look up the message.
Line 143: * @return The associated message in the passed in locale.
Line 144: */
Line 145: public static String getMessage(final String key, final Locale
locale) {
Why is this method static? Only singleton access methods like getInstance()
should be static.
Line 146: String result = "";
Line 147: // key needs to start with obrand.
Line 148: if (key != null && key.startsWith(BRAND_PREFIX + ".")) {
Line 149: String[] splitString = key.split("\\.");
Line 146: String result = "";
Line 147: // key needs to start with obrand.
Line 148: if (key != null && key.startsWith(BRAND_PREFIX + ".")) {
Line 149: String[] splitString = key.split("\\.");
Line 150: String prefix = null;
I'd use ternary operator here, for example:
String prefix = (splitString.length >= 2) ? ... : ...
Line 151: if (splitString.length >= 2) {
Line 152: prefix = key.split("\\.")[1];
Line 153: }
Line 154: if (prefix != null && prefix.length() > 0) {
Line 148: if (key != null && key.startsWith(BRAND_PREFIX + ".")) {
Line 149: String[] splitString = key.split("\\.");
Line 150: String prefix = null;
Line 151: if (splitString.length >= 2) {
Line 152: prefix = key.split("\\.")[1];
Why are we doing key.split("\\.") again, when we can just use splitString?
Line 153: }
Line 154: if (prefix != null && prefix.length() > 0) {
Line 155: result = getInstance().getMessageMap(prefix, locale).
Line 156: get(key.substring(key.indexOf(prefix) +
prefix.length() + 1));
Line 151: if (splitString.length >= 2) {
Line 152: prefix = key.split("\\.")[1];
Line 153: }
Line 154: if (prefix != null && prefix.length() > 0) {
Line 155: result = getInstance().getMessageMap(prefix, locale).
Since this method should be non-static, getInstance() should be redundant here.
Line 156: get(key.substring(key.indexOf(prefix) +
prefix.length() + 1));
Line 157: }
Line 158: }
Line 159: return result;
Line 164: * @param prefix The prefix to use for getting the keys.
Line 165: * @param locale The locale to get the messages for.
Line 166: * @return A {@code Map} of keys and values.
Line 167: */
Line 168: private Map<String, String> getMessageMap(final String prefix,
final Locale locale) {
This map is created and populated on each getMessage() invocation, did you
consider creating it only once and caching results for prefix + locale as cache
key?
Line 169: List<BrandingTheme> messageThemes = getBrandingThemes();
Line 170: // We need this map to remove potential duplicate strings
from the resource bundles.
Line 171: Map<String, String> keyValues = new HashMap<String, String>();
Line 172: if (messageThemes != null) {
Line 205: * @param keyValues The map to turn into the string.
Line 206: * @return A string of format {"key":"value",...}
Line 207: */
Line 208: String getMessagesFromMap(final Map<String, String> keyValues) {
Line 209: ObjectNode node = new ObjectMapper().createObjectNode();
Why create ObjectMapper on each invocation, instead of having ObjectMapper
field that gets initialized via (package-private) constructor?
Line 210: for (Map.Entry<String, String> entry : keyValues.entrySet()) {
Line 211: node.put(entry.getKey(), entry.getValue());
Line 212: }
Line 213: return node.size() > 0 ? node.toString() : null;
--
To view, visit http://gerrit.ovirt.org/15579
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I87f5a6684701c9d5f603bf79459a3a5b6614b24f
Gerrit-PatchSet: 5
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Alexander Wels <aw...@redhat.com>
Gerrit-Reviewer: Alexander Wels <aw...@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <alo...@redhat.com>
Gerrit-Reviewer: Einav Cohen <eco...@redhat.com>
Gerrit-Reviewer: Greg Sheremeta <gsher...@redhat.com>
Gerrit-Reviewer: Michael Pasternak <mpast...@redhat.com>
Gerrit-Reviewer: Vojtech Szocs <vsz...@redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
