Alon Bar-Lev has posted comments on this change.
Change subject: userportal, webadmin: branding support[WIP].
......................................................................
Patch Set 6: (2 inline comments)
....................................................
File
frontend/webadmin/modules/frontend/src/main/java/org/ovirt/engine/ui/frontend/server/gwt/branding/BrandingTheme.java
Line 108: path = brandingPath.substring(
Line 109: brandingRootPath.getAbsolutePath().
Line 110: length());
Line 111: filePath = brandingPath;
Line 112: available = true;
> You always assume failure, and set the result to success at the end. I assume
> success and only change the status in case of failure. That is an identical
> pattern just reversed.
Well, not exactly, as if you have another catch, you need another
available=false statement and if you forget it, you get wrong behavior, and if
you don't, this is useless statement. This is why assuming failure is safest
shortest.
> I don't like using exceptions for flow control which is more or less what
> your third suggestion is doing.
But this is exactly what exceptions are for... having a file missing is
exceptional, it is not normal flow. Exceptions were introduce to reduce the
number of conditionals in code, and manage these conditions within different
scope/flow.
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 tend to prioritize by:
1. beautiful, easy to read, self explainatory code.
2. performance.
Going from 1 to 2 happens only if there is a good reason (proof) that code hunk
is a performance bottle neck, using profiler for example. This may happen only
if a code is called in a loop at order of more than N.
This method is never called... and if it is it probably once in runtime. There
is no reason no optimize it.
But these are just my two cents... :)
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
