Mike Kolesnik has posted comments on this change. Change subject: core: extract current implementation to strategy. ......................................................................
Patch Set 5: (2 comments) http://gerrit.ovirt.org/#/c/26400/5/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/MacPoolManagerOriginal.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/MacPoolManagerOriginal.java: Line 133: Line 134: if (macAddresses.size() + availableMacs.size() > maxMacsInPool) { Line 135: availableMacs.addAll(macAddresses.subList(0, maxMacsInPool - availableMacs.size())); Line 136: throw new MacPoolExceededMaxException(); //TODO MM: what's this? why returning boolean value then? Line 137: } else { > there's semantic difference between 'nested else' and 'guard' (return/throw Since I don't have or read that book, it would be helpful if you supply the whole argument and not just citation. Anyway, IMHO the "if" condition matches the definition of "guard block" since it's not the normal path of method execution here, so makes sense not to have the else clause. Line 138: availableMacs.addAll(macAddresses); Line 139: // System.out.println("!!"+availableMacs.size()); Line 140: return true; Line 141: } Line 175: } Line 176: return true; Line 177: } Line 178: Line 179: void logMacPoolEmpty() { > yes. Please document this choice. Line 180: AuditLogableBase logable = new AuditLogableBase(); Line 181: AuditLogDirector.log(logable, AuditLogType.MAC_POOL_EMPTY); Line 182: } Line 183: -- To view, visit http://gerrit.ovirt.org/26400 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I75498bb68ec6dbe28650027116bf44a0797faf93 Gerrit-PatchSet: 5 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: Yevgeny Zaspitsky <[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
