Martin Mucha has posted comments on this change. Change subject: core: extract current implementation to strategy. ......................................................................
Patch Set 5: (2 comments) answers. 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 { > Since I don't have or read that book, it would be helpful if you supply the you know the answer even without book. Both is correct. If condition is observed as guard, then there is no else. If it's observed as a condition to decide yes/no, then there should be else. This is highly subjective, and I do strongly believe, that stuff this low-level and this pointless from the users of this class, is to be decided by author. I do prefer specifying else statement, so future modifications of methods cannot that easily introduce error. Mine opinion. Highly subjective. 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() { > Please document this choice. this class is removed in following patches (cannot be done here), same method on alternative implementation is made private on following patches. 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
