Yevgeny Zaspitsky has posted comments on this change. Change subject: webadmin: update Linq.NetworkComparator ......................................................................
Patch Set 16: (1 comment) http://gerrit.ovirt.org/#/c/37027/16/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/Linq.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/Linq.java: Line 1324: Line 1325: final boolean managementNetwork1 = net1.getCluster().isManagement(); Line 1326: final boolean managementNetwork2 = net2.getCluster().isManagement(); Line 1327: Line 1328: if (managementNetwork1 == managementNetwork2) { > Because using "==" could imply that there's a case where mangementNetwork1 Your comment implies that the comparator is (and will be) used for comparing Network objects with the same cluster, whereas that could not be guaranteed by the class. Moreover you assume additional logic that a single management network could exists in that single cluster. I'd refrain from assuming additional logic when coding the comparating logic and would stick to as narrow as possible aspect of it. Line 1329: return lexoNumeric.compare(net1.getName(), net2.getName()); Line 1330: } else { Line 1331: return managementNetwork1 ? -1 : 1; Line 1332: } -- To view, visit http://gerrit.ovirt.org/37027 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ib6d3c01b552c6a8034652e533d2bae7319cbd9fb Gerrit-PatchSet: 16 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Yevgeny Zaspitsky <yzasp...@redhat.com> Gerrit-Reviewer: Lior Vernia <lver...@redhat.com> Gerrit-Reviewer: Yevgeny Zaspitsky <yzasp...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
