Mike Kolesnik has posted comments on this change.

Change subject: engine: Introduce VnicProfileValidator
......................................................................


Patch Set 22: (3 inline comments)

....................................................
File 
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/vm/UpdateVnicProfileCommand.java
Line 18:     protected boolean canDoAction() {
Line 19:         VnicProfileValidator validator = new 
VnicProfileValidator(getVnicProfile());
Line 20:         return validate(validator.vnicProfileIsSet())
Line 21:                 && validate(validator.vnicProfileExists())
Line 22:                 && validate(validator.networkExists())
Why is it necessary?

If you validate that the profile exists, and that the network didn't change, 
then what good would checking if the network exists would bring here?
Line 23:                 && validate(validator.vnicProfileNameNotUsed())
Line 24:                 && validate(validator.networkNotChanged());
Line 25:     }
Line 26: 


....................................................
File 
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/validator/VnicProfileValidator.java
Line 40:                 ? new 
ValidationResult(VdcBllMessages.VNIC_PROFILE_NOT_EXISTS)
Line 41:                 : ValidationResult.VALID;
Line 42:     }
Line 43: 
Line 44:     public ValidationResult networkExists() {
This is part of NetworkValidator?!
Line 45:         return getNetwork() == null
Line 46:                 ? new 
ValidationResult(VdcBllMessages.NETWORK_NOT_EXISTS)
Line 47:                 : ValidationResult.VALID;
Line 48:     }


Line 109:     }
Line 110: 
Line 111:     protected List<VM> getVmsUsingProfile() {
Line 112:         if (vms == null) {
Line 113:             vms = 
getDbFacade().getVmDao().getAllForVnicProfile(vnicProfile.getId());
Why are you caching this list?

You don't seem to be reusing it...
Also you're not caching the templates list so I'm not following the consistency 
here..
Line 114:         }
Line 115: 
Line 116:         return vms;
Line 117:     }


-- 
To view, visit http://gerrit.ovirt.org/16887
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I6af74a398d1d7ffb97a1802305c56ad536acb346
Gerrit-PatchSet: 22
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Moti Asayag <masa...@redhat.com>
Gerrit-Reviewer: Alona Kaplan <alkap...@redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches

Reply via email to