Arik Hadas has posted comments on this change. Change subject: core: command for updating template nics. ......................................................................
Patch Set 6: (2 comments) my comments also hold for the previous implementation of UpdateAndReorderVmNicsCommand, but it is an opportunity to fix them. in general, it looks ok http://gerrit.ovirt.org/#/c/35285/6/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AbstractUpdateNicsCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AbstractUpdateNicsCommand.java: Line 39: UpdateNicsVmInterfaceParametersFactory parameters = calculateRequiredParameters(getExistingNics()); Line 40: Line 41: invokeActionType(VdcActionType.AddVmInterface, parameters.createVnicParameters); Line 42: invokeActionType(VdcActionType.UpdateVmInterface, parameters.updateVnicParameters); Line 43: invokeActionType(VdcActionType.RemoveVmInterface, parameters.removeVnicParameters); IMO, this is a weird usage of 'factory class'. To conform the standard way of using the factory pattern I suggest that calculateRequiredParameters will be renamed to create...Factory and will just return the factory class (without calling createParameters method). here, you'll call createCreateVnicParameters(), createUpdateVnicParameters() and createRemoveVnicParameters(). inside the factory class you can produce the needed data on the instantiation of the factory class and just return it in those methods, but from the API point of view it will be more similar to standard factory classes in java Line 44: Line 45: if (getParameters().reorderNics) { Line 46: VmOperationParameterBase reorderParams = new VmOperationParameterBase(getParameters().vmId); Line 47: runInternalAction(VdcActionType.ReorderVmNics, reorderParams); Line 50: s/getReturnValue().setSucceeded(true);/setSucceeded(true);/g -- To view, visit http://gerrit.ovirt.org/35285 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ibf67d6e0aeb094859f7eb1930bb151164f5207be Gerrit-PatchSet: 6 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Martin Mucha <mmu...@redhat.com> Gerrit-Reviewer: Arik Hadas <aha...@redhat.com> Gerrit-Reviewer: Lior Vernia <lver...@redhat.com> Gerrit-Reviewer: Martin Mucha <mmu...@redhat.com> Gerrit-Reviewer: Moti Asayag <masa...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
