Mike Kolesnik has posted comments on this change.
Change subject: core: Add mom policy update command
......................................................................
Patch Set 5: (7 inline comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/UpdateMomPolicyCommand.java
Line 27: public UpdateMomPolicyCommand(VdsActionParameters
vdsActionParameters) {
Line 28: super(vdsActionParameters);
Line 29: }
Line 30:
Line 31: private boolean runUpdateMomPolicy(final VDS vds, boolean
ballooningEnabled) {
Not sure why this is necessary as a separate method, as the execute simply
calls it.
IMHO this method should be inlined to the executeCommand method
Line 32: boolean succeeded = false;
Line 33: try {
Line 34: succeeded =
runVdsCommand(VDSCommandType.SetMOMPolicyParameters,
Line 35: new MomPolicyVDSParameters(vds,
ballooningEnabled)).getSucceeded();
Line 48: protected boolean canDoAction() {
Line 49: VdsValidator vdsValidator = new VdsValidator(getVds());
Line 50:
Line 51: ValidationResult exists = vdsValidator.exists();
Line 52: if (!exists.isValid()) {
You can write this CDA code more simply with validators:
return validate(vdsValidator.exists())
&& validate(vdsValidator.validateStatus(VDSStatus.Up));
Line 53: return failCanDoAction(exists.getMessage());
Line 54: }
Line 55:
Line 56: ValidationResult up =
vdsValidator.validateStatus(VDSStatus.Up);
Line 68: }
Line 69:
Line 70: @Override
Line 71: public AuditLogType getAuditLogTypeValue() {
Line 72: return getSucceeded() ?
Default formatter would put the ? and : in the beginning of the next line..
This actually makes sense as it makes reading the code more fluid - as the line
breaks, you can see by it what is it's purpose - is it the code when true (?)
or false(:) and you don't have to look up this info on the previous line
Line 73: AuditLogType.USER_UPDATED_MOM_POLICIES :
Line 74: AuditLogType.USER_FAILED_TO_UPDATE_MOM_POLICIES;
Line 75: }
....................................................
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()) {
Why check here implicitly if the host exists?
Either it was already checked by whomever is using this validator, or he just
would fail with NPE indicating a bug so either way there is no reason to do
this here..
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()) {
it is prefferrable to use .equals() for this check since we're dealing with
objects..
Line 91: return new
ValidationResult(VdcBllMessages.ACTION_TYPE_FAILED_VDS_STATUS_ILLEGAL);
Line 92: }
Line 93: return ValidationResult.VALID;
Line 94: }
Line 87: if (!existsValidation.isValid()) {
Line 88: return existsValidation;
Line 89: }
Line 90: if (vdsStatus != vds.getStatus()) {
Line 91: return new
ValidationResult(VdcBllMessages.ACTION_TYPE_FAILED_VDS_STATUS_ILLEGAL);
Please notice that the translation for this key requires a replacement for
${hostStatus}..
I suggest to receive the status as a parameter to this method.
Furthermore, you could add another method, for convenience, such as:
public ValidationResult isUp() {
return validateStatus(VDSStatus.Up, VdcBllMessages.VAR__HOST_STATUS__UP);
}
This method would be nice to have as it would prevent further duplication and
make the code easier to read..
Line 92: }
Line 93: return ValidationResult.VALID;
Line 94: }
Line 89: }
Line 90: if (vdsStatus != vds.getStatus()) {
Line 91: return new
ValidationResult(VdcBllMessages.ACTION_TYPE_FAILED_VDS_STATUS_ILLEGAL);
Line 92: }
Line 93: return ValidationResult.VALID;
The whole code can be simplified as:
return vdsStatus.equals(vds.getStatus())
? ValidationResult.VALID
: new
ValidationResult(VdcBllMessages.ACTION_TYPE_FAILED_VDS_STATUS_ILLEGAL,
statusReplacement);
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
