Omer Frenkel has posted comments on this change.
Change subject: Refactor UpdateVdsGroupCommand
......................................................................
Patch Set 3: (6 inline comments)
i really don't like the way some methods return true in the middle of it, as it
is hard to understand and harder to maintenance,
for example, if i would like to add a check in validateMemberVmsState method,
i should be really carefull where i put my code, as it will not always be
checked!
on the other hand, methods like validateCpuFlagsChange and
validateCompatibilityVersionChange are easy to understand and very easy to add
checks if needed. (BTW, adding validation in the beggining like in
validateCpuChange is acceptable, IMHO)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/UpdateVdsGroupCommand.java
Line 155:
actionMessages.add(VdcBllMessages.VAR__ACTION__UPDATE.toString());
you can just use here addCanDoActionMessage(VdcBllMessages.VAR__ACTION__UPDATE)
Line 186: }
since you did the great job of refactoring to methods,
cleaner code would be to call these together:
if (!(validateCluster(oldGroup) && validateGroupNameChange(oldGroup) &&
validateCpuChange(oldGroup) ...) {
return false;
}
Line 193: if (!validateLocalfsSpecificRequirements()) {
why not add these 2 ifs together?
Line 225: .toString());
use addCanDoActionMessage
Line 234: .toString());
use addCanDoActionMessage
Line 343: ArrayList<String> actionMessages =
getReturnValue().getCanDoActionMessages();
please use addCanDoActionMessage instead of keeping this member
--
To view, visit http://gerrit.ovirt.org/2271
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I47300fec2f6f5fabdbc75e8c78332c5aef2508a2
Gerrit-PatchSet: 3
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Saggi Mizrahi <[email protected]>
Gerrit-Reviewer: Ayal Baron <[email protected]>
Gerrit-Reviewer: Haim Ateya <[email protected]>
Gerrit-Reviewer: Maor Lipchuk <[email protected]>
Gerrit-Reviewer: Omer Frenkel <[email protected]>
Gerrit-Reviewer: Saggi Mizrahi <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
