Vojtech Szocs has posted comments on this change.
Change subject: HostGeneralModel: improve "node has upgrade" logic
......................................................................
Patch Set 2: (1 inline comment)
....................................................
File
frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/hosts/HostGeneralModel.java
Line 686: Date dateISO = format.parse(formattedISOVersion);
Line 687: // Split by . nodeInfo[2] and get the first element.
Example: 20120228.fc18
Line 688: Date dateNodeISO =
format.parse(nodeInfo[2].split("\\.")[0].trim()); //$NON-NLS-1$
Line 689: if (dateISO.compareTo(dateNodeISO)>0) {
Line 690: alert = true;
Guys, I suggest taking a look at
http://discuss.joelonsoftware.com/default.asp?joel.3.325456.34
Personally, I agree with following quote, taken from above mentioned forum:
<quote>
The only valid reason I've ever seen for single exit points is that it makes
resource deallocation simpler. One place to exit = one place to free (in
theory).
Personally I don't agree with the philosophy. I think that single exit points
can cause readability issues. I think the code is actually /harder/ to trace,
because it has to be oddly structured to get everything down to the bottom
without executing incorrect code paths (e.g. statement 1 failed, but you need
to get past statements 2 and 3 to get to the exit point).
I do think that exit points should be minimized, but then, I also think
function size should be minimized. If you've got a dozen exit points, the
problem isn't just that you need to change them all into a single exit point.
It's that you need to fix your whole function.
</quote>
I'm not against single exit point in general, but in this case, I feel it adds
extra code which I see as redundant.
However, I'm also OK with current approach, it was just my suggestion :-)
Line 691: break;
Line 692: }
Line 693: } catch (IllegalArgumentException er) {
Line 694: logger.info("IllegalArgumentException error: " + er);
//$NON-NLS-1$
--
To view, visit http://gerrit.ovirt.org/16089
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I49c74d3d0d92c74848e0c175095610b6e6b5a88e
Gerrit-PatchSet: 2
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Douglas Schilling Landgraf <[email protected]>
Gerrit-Reviewer: Alexander Wels <[email protected]>
Gerrit-Reviewer: Barak Azulay <[email protected]>
Gerrit-Reviewer: Doron Fediuck <[email protected]>
Gerrit-Reviewer: Douglas Schilling Landgraf <[email protected]>
Gerrit-Reviewer: Einav Cohen <[email protected]>
Gerrit-Reviewer: Vojtech Szocs <[email protected]>
Gerrit-Reviewer: oVirt Jenkins CI Server
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
