Alon Bar-Lev has posted comments on this change.
Change subject: userportal, webadmin: branding support[WIP].
......................................................................
Patch Set 6: (3 inline comments)
....................................................
File
frontend/webadmin/modules/frontend/src/main/java/org/ovirt/engine/ui/frontend/server/gwt/branding/BrandingTheme.java
Line 64:
Line 65: /**
Line 66: * The key of the web admin css in the branding properties.
Line 67: */
Line 68: private static final String WEB_ADMIN_CSS_KEY = "web_admin_css";
//$NON-NLS-1$
Can't we derive this from the above enum?
Line 69:
Line 70: /**
Line 71: * The key for the resource bundle name.
Line 72: */
Line 108: path = brandingPath.substring(
Line 109: brandingRootPath.getAbsolutePath().
Line 110: length());
Line 111: filePath = brandingPath;
Line 112: available = true;
Some generic design pattern notes... while we at it... :)
I tend to always assume failure... as a pattern. this avoids mistakes when
handling multiple failures...
In this case:
available = false;
try {
whatever
available = true;
}
catch (...) {
...
}
However... instead of:
xxx = new XXX(parameters);
if (xxx.available()) {
add(xxx);
}
It should be better to:
xxx = new XXX();
if (xxx.load(parameters)) {
add(xxx);
}
as you don't fail the constructor, and can defer the method that can fail.
or better yet... use exceptional patterns.... instead of return value pattern:
try {
add(new XXX(parameters)).
}
catch(RuntimeException e) {
log.warn(e)
}
this means you throw exception out of the constructor and remove the logging or
change it to debug. This is the best method, as class is not really usable if
constructor fails.
Line 113: FileInputStream propertiesFile;
Line 114: try {
Line 115: propertiesFile = new FileInputStream(brandingPath
Line 116: + "/" + BRANDING_PROPERTIES_NAME); //$NON-NLS-1$
Line 219: builder.append(getPath());
Line 220: builder.append(", User portal css: "); //$NON-NLS-1$
Line 221: builder.append(getUserPortalStyleSheetName());
Line 222: builder.append(", Web admin css: "); //$NON-NLS-1$
Line 223: builder.append(getWebadminStyleSheetName());
I know there was a discussion around this... but....
return String.format(
"Path to theme %1$s, User portal css: %2$s, Web admin css: %3$s",
getPath(),
getUserPortalStyleSheetName(),
getWebadminStyleSheetName()
);
Much more readable!
Line 224: return builder.toString();
Line 225: }
--
To view, visit http://gerrit.ovirt.org/13181
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I4a8a426ce7d688d33c5ae2b70632c836843106b2
Gerrit-PatchSet: 6
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Alexander Wels <aw...@redhat.com>
Gerrit-Reviewer: Alex Lourie <alou...@redhat.com>
Gerrit-Reviewer: Alexander Wels <aw...@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <alo...@redhat.com>
Gerrit-Reviewer: Daniel Erez <de...@redhat.com>
Gerrit-Reviewer: Einav Cohen <eco...@redhat.com>
Gerrit-Reviewer: Eyal Edri <ee...@redhat.com>
Gerrit-Reviewer: Itamar Heim <ih...@redhat.com>
Gerrit-Reviewer: Kanagaraj M <kmayi...@redhat.com>
Gerrit-Reviewer: Moran Goldboim <mgold...@redhat.com>
Gerrit-Reviewer: Sahina Bose <sab...@redhat.com>
Gerrit-Reviewer: Sandro Bonazzola <sbona...@redhat.com>
Gerrit-Reviewer: Shireesh Anjal <san...@redhat.com>
Gerrit-Reviewer: Vojtech Szocs <vsz...@redhat.com>
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
