Alissa Bonas has posted comments on this change.
Change subject: core: Change CommandBase validate method to accept multiple
results
......................................................................
Patch Set 1: (2 inline comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddDiskCommand.java
Line 142:
Line 143: if (returnValue && vm != null) {
Line 144: storage_pool sp = getStoragePool(); // Note this is done
according to the VM's spId.
Line 145: returnValue =
Line 146: validate(new StoragePoolValidator(sp).isUp()) &&
I am definitely in favour of unifying all the multiple validates as a general
concept.
However this specific piece of code , is , in my opinion still hard to read.
Still too many "&&", passing method calls inside the validate method call (
hard to debug), need to count here all the parameters passed, it's not clear
when the method call definition ends with all the brackets . For me , it's too
complicated to read and maintain such code.
Also there are 2 validate calls here made together - not clear why.
Line 147: isStoragePoolMatching(vm) &&
Line 148:
performImagesChecks(vm.getStoragePoolId()) &&
Line 149:
validate(getSnapshotValidator().vmNotDuringSnapshot(vm.getId()),
Line 150:
getSnapshotValidator().vmNotInPreview(vm.getId()),
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/CommandBase.java
Line 1499: * @param validationResults
Line 1500: * The validation results from the inline call to
validate, should not be <code>null</code>.
Line 1501: * @return <code>true</code> if the validations were successful,
and <code>false</code> if one or more wasn't.
Line 1502: */
Line 1503: protected boolean validate(ValidationResult...
validationResults) {
personally (and I guess it's a matter of style preference) I don't like the
syntax of the "three dots" because it's not clear enough what and how much do
you expect to receive in the function - I prefer in such cases getting one
parameter which can be a collection/set/map/list.
Line 1504: for (ValidationResult validationResult : validationResults) {
Line 1505: if (!validationResult.isValid()) {
Line 1506: addCanDoActionMessage(validationResult.getMessage());
Line 1507: if (validationResult.getVariableReplacements() !=
null) {
--
To view, visit http://gerrit.ovirt.org/11557
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I64a18ec0e6c677192dbd86bf0b38de46e539d138
Gerrit-PatchSet: 1
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Tal Nisan <tni...@redhat.com>
Gerrit-Reviewer: Alissa Bonas <abo...@redhat.com>
Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com>
Gerrit-Reviewer: Ayal Baron <aba...@redhat.com>
Gerrit-Reviewer: Maor Lipchuk <mlipc...@redhat.com>
Gerrit-Reviewer: Tal Nisan <tni...@redhat.com>
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
