Martin Mucha has uploaded a new change for review.
Change subject: userportal,webadmin: repetitive instanceof checks in one big
method traded off for two rather similar methods.
......................................................................
userportal,webadmin: repetitive instanceof checks in one big method traded off
for two rather similar methods.
this adds some 'duplicity' (which probably can be 'dealt with' with
proper design) among two methods, but drops irrelevant code from being
processed. Also (potential) adding of new operand type would led to
instanceof paradise.
Change-Id: I08cf6c098798dcd17a46310b070445d1c983124d
Signed-off-by: Martin Mucha <mmu...@redhat.com>
---
M
frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/hosts/network/NetworkOperationFactory.java
1 file changed, 73 insertions(+), 58 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/ovirt-engine refs/changes/42/37642/1
diff --git
a/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/hosts/network/NetworkOperationFactory.java
b/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/hosts/network/NetworkOperationFactory.java
index de7fd96..e6f011e 100644
---
a/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/hosts/network/NetworkOperationFactory.java
+++
b/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/hosts/network/NetworkOperationFactory.java
@@ -84,13 +84,24 @@
private static NetworkOperation handleBinaryOperation(NetworkItemModel<?>
op1, NetworkItemModel<?> op2) {
// binary operation joining items together - in most cases valid iff
their networks comply
if (op2 instanceof NetworkInterfaceModel) {
- return
binaryOperationWithNetworkInterfaceModelAsSecondOperand(op1,
(NetworkInterfaceModel) op2);
+ if (op1 instanceof NetworkInterfaceModel) {
+ return
binaryOperationWithNetworkInterfaceModelAndNetworkInterfaceModelOperands(
+ (NetworkInterfaceModel) op1,
+ (NetworkInterfaceModel) op2);
+ }
+
+ // op1 is a network, verify that it isn't dragged unto the NIC
already containing it
+ if (op1 instanceof LogicalNetworkModel) {
+ return
binaryOperationWithLogicalNetworkModelAndNetworkInterfaceModelOperands(
+ (LogicalNetworkModel) op1, (NetworkInterfaceModel)
op2);
+ }
}
return NetworkOperation.NULL_OPERATION;
}
- private static NetworkOperation
binaryOperationWithNetworkInterfaceModelAsSecondOperand(NetworkItemModel<?> op1,
+ private static NetworkOperation
binaryOperationWithNetworkInterfaceModelAndNetworkInterfaceModelOperands(
+ NetworkInterfaceModel op1,
NetworkInterfaceModel dst) {
// first collect the networks into one set
@@ -98,20 +109,10 @@
networks.addAll(dst.getItems());
// op1 is a NIC, verify that it isn't already part of a bond or
dragged unto itself
- if (op1 instanceof NetworkInterfaceModel) {
- NetworkInterfaceModel src = (NetworkInterfaceModel) op1;
- if (src.isBonded() || src.equals(dst)) { //TODO MM: is there a
place where 'A op A' is valid?
- return NetworkOperation.NULL_OPERATION;
- } else {
- networks.addAll(((NetworkInterfaceModel) op1).getItems());
- }
+ if (op1.isBonded() || op1.equals(dst)) { //TODO MM: is there a
place where 'A op A' is valid?
+ return NetworkOperation.NULL_OPERATION;
} else {
- // op1 is a network, verify that it isn't dragged unto the NIC
already containing it
- if (op1 instanceof LogicalNetworkModel) {
- if (!networks.add((LogicalNetworkModel) op1)) {
- return NetworkOperation.NULL_OPERATION;
- }
- }
+ networks.addAll(op1.getItems());
}
// go over the networks and check whether they comply, if not - the
reason is important
@@ -120,24 +121,12 @@
int nonVlanCounter = 0;
for (LogicalNetworkModel network : networks) {
if (!network.isManaged()) {
- if (op1 instanceof LogicalNetworkModel) {
- return NetworkOperation.NULL_OPERATION_UNMANAGED;
- }
-
- if (op1 instanceof NetworkInterfaceModel) {
- dst.setCulpritNetwork(network.getName());
- return NetworkOperation.NULL_OPERATION_BOND_UNMANAGED;
- }
+ dst.setCulpritNetwork(network.getName());
+ return NetworkOperation.NULL_OPERATION_BOND_UNMANAGED;
} else {
if (!network.isInSync()) {
- if (op1 instanceof LogicalNetworkModel) {
- return NetworkOperation.NULL_OPERATION_OUT_OF_SYNC;
- }
-
- if (op1 instanceof NetworkInterfaceModel) {
- dst.setCulpritNetwork(network.getName());
- return
NetworkOperation.NULL_OPERATION_BOND_OUT_OF_SYNC;
- }
+ dst.setCulpritNetwork(network.getName());
+ return NetworkOperation.NULL_OPERATION_BOND_OUT_OF_SYNC;
}
}
@@ -151,31 +140,14 @@
}
if (nonVlanCounter > 1) {
- if (op1 instanceof LogicalNetworkModel) {
- return NetworkOperation.NULL_OPERATION_TOO_MANY_NON_VLANS;
- }
-
- if (op1 instanceof NetworkInterfaceModel) {
- dst.setCulpritNetwork(network.getName());
- return
NetworkOperation.NULL_OPERATION_BOND_TOO_MANY_NON_VLANS;
- }
+ dst.setCulpritNetwork(network.getName());
+ return NetworkOperation.NULL_OPERATION_BOND_TOO_MANY_NON_VLANS;
} else {
if (nonVlanVmNetwork != null && vlanFound) {
- if (op1 instanceof LogicalNetworkModel) {
- return NetworkOperation.NULL_OPERATION_VM_WITH_VLANS;
- }
-
- if (op1 instanceof NetworkInterfaceModel) {
- dst.setCulpritNetwork(nonVlanVmNetwork);
- return
NetworkOperation.NULL_OPERATION_BOND_VM_WITH_VLANS;
- }
+ dst.setCulpritNetwork(nonVlanVmNetwork);
+ return NetworkOperation.NULL_OPERATION_BOND_VM_WITH_VLANS;
}
}
- }
-
- // networks comply, all that's left is to return the correct operation
- if (op1 instanceof LogicalNetworkModel) {
- return NetworkOperation.ATTACH_NETWORK;
}
if (op1 instanceof BondNetworkInterfaceModel) {
@@ -186,16 +158,59 @@
}
}
- if (op1 instanceof NetworkInterfaceModel) {
- if (dst instanceof BondNetworkInterfaceModel) {
- return NetworkOperation.ADD_TO_BOND;
+ if (dst instanceof BondNetworkInterfaceModel) {
+ return NetworkOperation.ADD_TO_BOND;
+ } else {
+ return NetworkOperation.BOND_WITH;
+ }
+ }
+
+ private static NetworkOperation
binaryOperationWithLogicalNetworkModelAndNetworkInterfaceModelOperands(
+ LogicalNetworkModel op1,
+ NetworkInterfaceModel dst) {
+
+ // first collect the networks into one set
+ Set<LogicalNetworkModel> networks = new HashSet<>();
+ networks.addAll(dst.getItems());
+
+ // op1 is a network, verify that it isn't dragged unto the NIC already
containing it
+ if (!networks.add(op1)) {
+ return NetworkOperation.NULL_OPERATION;
+ }
+
+ // go over the networks and check whether they comply, if not - the
reason is important
+ boolean vlanFound = false;
+ String nonVlanVmNetwork = null;
+ int nonVlanCounter = 0;
+ for (LogicalNetworkModel network : networks) {
+ if (!network.isManaged()) {
+ return NetworkOperation.NULL_OPERATION_UNMANAGED;
} else {
- return NetworkOperation.BOND_WITH;
+ if (!network.isInSync()) {
+ return NetworkOperation.NULL_OPERATION_OUT_OF_SYNC;
+ }
+ }
+
+ if (network.hasVlan()) {
+ vlanFound = true;
+ } else {
+ ++nonVlanCounter;
+ if (network.getEntity().isVmNetwork()) {
+ nonVlanVmNetwork = network.getName();
+ }
+ }
+
+ if (nonVlanCounter > 1) {
+ return NetworkOperation.NULL_OPERATION_TOO_MANY_NON_VLANS;
+ } else {
+ if (nonVlanVmNetwork != null && vlanFound) {
+ return NetworkOperation.NULL_OPERATION_VM_WITH_VLANS;
+ }
}
}
- return NetworkOperation.NULL_OPERATION;
-
+ // networks comply, all that's left is to return the correct operation
+ return NetworkOperation.ATTACH_NETWORK;
}
private static boolean noValidOperationForFirstOperand(NetworkItemModel<?>
op1) {
--
To view, visit http://gerrit.ovirt.org/37642
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I08cf6c098798dcd17a46310b070445d1c983124d
Gerrit-PatchSet: 1
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Martin Mucha <mmu...@redhat.com>
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
