Martin Mucha has posted comments on this change. Change subject: core: MacPool related Commands ......................................................................
Patch Set 11: (3 comments) answers 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(); > Well since you asked I won't start with the premature optimization but rath I'm inlining it. Below is explanation from which perspective it's former actually ok. Read about DRY and "clean code" if interrested. From THAT perspective is OK to create variable, event for purpose just to give a name to statement to help reader understand code. Example: int secsInDay = 24*60*60; is a proper way, although it may offend bright minded people, who will inline 86400 primitive. method call should not expect anything about it's implementation details; method can be potentially costly and it's content changed without notifying callers. So just like in conversation you don't ask for person's name every time you want to say hello, it's better, from proper OO perspective, not to ask for same thing many times. Most of time is just a slight improvement , most of time readability improvement, and safety net for cases when multiple calls can cause different result. Technically it's pointless and only matter of style of clean code. I'm not delving into semantical meaning of variable, it's just a pointer to piece of memory to be used for your intention. Maybe different languagues have different rules, but JAVA is commonly used with dry, clean coding, and without LOC/manual gzip optimizations. 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(); > I disagree that "Code's cleaner and better this way", it's highly speculati Done 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), > They have meaning since they're saved to DB. The range itself is meaningles Done 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: Lior Vernia <[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
