Mike Kolesnik has posted comments on this change.
Change subject: core: Add mom policy update command
......................................................................
Patch Set 5: (2 inline comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VdsValidator.java
Line 83: }
Line 84:
Line 85: public ValidationResult validateStatus(VDSStatus vdsStatus) {
Line 86: ValidationResult existsValidation = exists();
Line 87: if (!existsValidation.isValid()) {
In this reasoning, null value should be valid one not invalid, per the
javax.validator doctrine.
I don't see why mix two checks into one, when one should be already performed
anyway.
IMHO It's not good for anything, it's just confusing.. You hide a possible bug
and make this method have multiple concerns when it should focus just on one
(the host status).
Line 88: return existsValidation;
Line 89: }
Line 90: if (vdsStatus != vds.getStatus()) {
Line 91: return new
ValidationResult(VdcBllMessages.ACTION_TYPE_FAILED_VDS_STATUS_ILLEGAL);
Line 86: ValidationResult existsValidation = exists();
Line 87: if (!existsValidation.isValid()) {
Line 88: return existsValidation;
Line 89: }
Line 90: if (vdsStatus != vds.getStatus()) {
They are wrong, it doesn't make sense and it's not OOP to use ==/!= to make
these checks.
Maybe if this wasn't Java but in Java == is not the same check as equals.
I won't -1 about this, but know that the people who advocate for using == check
for non-primitives probably don't understand basic OOP concepts and how they
are used in Java...
Line 91: return new
ValidationResult(VdcBllMessages.ACTION_TYPE_FAILED_VDS_STATUS_ILLEGAL);
Line 92: }
Line 93: return ValidationResult.VALID;
Line 94: }
--
To view, visit http://gerrit.ovirt.org/17379
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I64350c17f182c1dd0ba1d06130e5d56019b32fc9
Gerrit-PatchSet: 5
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: ofri masad <[email protected]>
Gerrit-Reviewer: Allon Mureinik <[email protected]>
Gerrit-Reviewer: Doron Fediuck <[email protected]>
Gerrit-Reviewer: Gilad Chaplik <[email protected]>
Gerrit-Reviewer: Martin Sivák <[email protected]>
Gerrit-Reviewer: Mike Kolesnik <[email protected]>
Gerrit-Reviewer: Roy Golan <[email protected]>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-Reviewer: ofri masad <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
