Moti Asayag has posted comments on this change.
Change subject: engine: Network Command Validation
......................................................................
Patch Set 2: (7 inline comments)
This is a great patch which makes the can-do-action readable easily and clearly.
Next step would be squashing all of the
If (!cond1()){
return false;
}
If (!cond2()){
return false;
}
with:
if (!cond1()
|| !cond2()
...
return false;
}
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/dc/AddNetworkCommand.java
Line 121: : new
ValidationResult(VdcBllMessages.NETWORK_NAME_ALREADY_EXISTS);
Line 122: }
Line 123:
Line 124: private Network getNetworkByName(List<Network> networks) {
Line 125: return LinqUtils.firstOrNull(networks, new
Predicate<Network>() {
LinqUtils could be replaced with for loop and return statement which will
simplify the reading and reduce the need of creating new anonymous class.
Line 126: @Override
Line 127: public boolean eval(Network network) {
Line 128: return network.getname().equals(getNetworkName());
Line 129: }
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/dc/NetworkCommon.java
Line 75: ? ValidationResult.VALID
Line 76: : new
ValidationResult(VdcBllMessages.NETWORK_CLUSTER_NETWORK_IN_USE);
Line 77: }
Line 78:
Line 79: protected ValidationResult networkNameStartsWithoutBond() {
would you consider changing the method name to networkPrefixValid() ?
since current name is too specific and suppose later on we'd encouter
additional problematic prefixes
Line 80: return getNetworkName().toLowerCase().startsWith("bond")
Line 81: ? new
ValidationResult(VdcBllMessages.NETWORK_CANNOT_CONTAIN_BOND_NAME)
Line 82: : ValidationResult.VALID;
Line 83: }
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/dc/UpdateNetworkCommand.java
Line 99: return super.getValidationGroups();
Line 100: }
Line 101:
Line 102: private Network getNetworkById(List<Network> networks) {
Line 103: return LinqUtils.firstOrNull(networks, new
Predicate<Network>() {
LinqUtils should be replaced with a plan for loop.
It can be done in a separate patch though.
Line 104: @Override
Line 105: public boolean eval(Network network) {
Line 106: return network.getId().equals(getNetwork().getId());
Line 107: }
Line 120: : ValidationResult.VALID;
Line 121: }
Line 122:
Line 123: private ValidationResult networkNameNotUsed(List<Network>
networks) {
Line 124: Network networkWithSameName = LinqUtils.firstOrNull(networks,
new Predicate<Network>() {
same comment regarding LinqUtils
Line 125: @Override
Line 126: public boolean eval(Network network) {
Line 127: return network.getname().trim().toLowerCase()
Line 128: .equals(getNetworkName().trim().toLowerCase())
Line 144: }
Line 145: }
Line 146: return ValidationResult.VALID;
Line 147: }
Line 148:
We have sort of 'hole' in RemoveNetworkCommand - seems we do not block removing
the management network.
This is out of this patch scope, but using this new method it would be done
easily, so please move this method to NetworkCommon and change its modifier to
be protected within this patch.
Line 149: private ValidationResult notDefaultNetwork(Network oldNetwork) {
Line 150: String defaultNetwork = Config.<String>
GetValue(ConfigValues.ManagementNetwork);
Line 151: return oldNetwork.getname().equals(defaultNetwork) &&
Line 152: !getNetworkName().equals(defaultNetwork)
Line 145: }
Line 146: return ValidationResult.VALID;
Line 147: }
Line 148:
Line 149: private ValidationResult notDefaultNetwork(Network oldNetwork) {
please change method name to 'notManagementNetwork' and also local parameter
name within the method.
Line 150: String defaultNetwork = Config.<String>
GetValue(ConfigValues.ManagementNetwork);
Line 151: return oldNetwork.getname().equals(defaultNetwork) &&
Line 152: !getNetworkName().equals(defaultNetwork)
Line 153: ? new
ValidationResult(VdcBllMessages.NETWORK_CAN_NOT_REMOVE_DEFAULT_NETWORK)
Line 147: }
Line 148:
Line 149: private ValidationResult notDefaultNetwork(Network oldNetwork) {
Line 150: String defaultNetwork = Config.<String>
GetValue(ConfigValues.ManagementNetwork);
Line 151: return oldNetwork.getname().equals(defaultNetwork) &&
please use oldNetwork.getName()
Line 152: !getNetworkName().equals(defaultNetwork)
Line 153: ? new
ValidationResult(VdcBllMessages.NETWORK_CAN_NOT_REMOVE_DEFAULT_NETWORK)
Line 154: : ValidationResult.VALID;
Line 155: }
--
To view, visit http://gerrit.ovirt.org/10542
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I4bb5bafd2029b3a8729bcf78d359804c6f587ff7
Gerrit-PatchSet: 2
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Muli Salem <msa...@redhat.com>
Gerrit-Reviewer: Mike Kolesnik <mkole...@redhat.com>
Gerrit-Reviewer: Moti Asayag <masa...@redhat.com>
Gerrit-Reviewer: Muli Salem <msa...@redhat.com>
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
