Mike Kolesnik has posted comments on this change. Change subject: core: MacPool related Commands ......................................................................
Patch Set 11: (3 comments) http://gerrit.ovirt.org/#/c/28705/11/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddMacPoolCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddMacPoolCommand.java: Line 62: Line 63: @Override Line 64: protected void executeCommand() { Line 65: this.macPoolDao = getDbFacade().getMacPoolDao(); Line 66: MacPool macPool = getMacPool(); > maybe I do not understand. Well since you asked I won't start with the premature optimization but rather with the fact that a variable serves a purpose of being variable, i.e. it's value can change. Here it's not a variable. Now, please explain why you need this to be saved in a local variable. Line 67: Line 68: macPoolDao.save(macPool); Line 69: Line 70: MacPoolPerDC.getInstance().createPool(macPool); http://gerrit.ovirt.org/#/c/28705/11/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveMacPoolCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveMacPoolCommand.java: Line 24: } Line 25: Line 26: @Override Line 27: protected void executeCommand() { Line 28: final Guid macPoolId = getParameters().getId(); > it's used twice. Code's cleaner and better this way. I disagree that "Code's cleaner and better this way", it's highly speculative and in this case I don't see how it makes code cleaner or better.. Line 29: Line 30: deletedMacPool = macPoolDao.get(getParameters().getId()); Line 31: macPoolDao.remove(macPoolId); Line 32: MacPoolPerDC.getInstance().removePool(macPoolId); http://gerrit.ovirt.org/#/c/28705/11/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/VdcActionType.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/VdcActionType.java: Line 365: // Audit Log Line 366: RemoveAuditLogById(2100, false, QuotaDependency.NONE), Line 367: ClearAllDismissedAuditLogs(2101, false, QuotaDependency.NONE), Line 368: Line 369: SetDataOnSession(3000, false, QuotaDependency.NONE), > a) caused by rebase They have meaning since they're saved to DB. The range itself is meaningless but it's good practice to keep it ordered to avoid confusion and duplication of IDs Line 370: Line 371: // Mac Pool Line 372: AddMacPool(2200, ActionGroup.CREATE_MAC_POOL, false, QuotaDependency.NONE), Line 373: UpdateMacPool(2201, ActionGroup.EDIT_MAC_POOL, false, QuotaDependency.NONE), -- To view, visit http://gerrit.ovirt.org/28705 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I9c0c3657e3e53699bcafa375befdce848b01a2f3 Gerrit-PatchSet: 11 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Martin Mucha <[email protected]> Gerrit-Reviewer: Martin Mucha <[email protected]> Gerrit-Reviewer: Mike Kolesnik <[email protected]> Gerrit-Reviewer: Moti Asayag <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
