Martin Mucha has posted comments on this change. Change subject: core: MacPool related Commands ......................................................................
Patch Set 27: (8 comments) http://gerrit.ovirt.org/#/c/28705/27/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/MacPoolValidator.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/MacPoolValidator.java: Line 61: return null; Line 62: } Line 63: Line 64: private ValidationResult testNameNullity() { Line 65: if (macPool.getName() == null) { > This should be done by javax.validation on the MacPool itself Done Line 66: return new ValidationResult(VdcBllMessages.ACTION_TYPE_FAILED_NAME_MAY_NOT_BE_EMPTY); Line 67: } else { Line 68: return null; Line 69: } Line 69: } Line 70: } Line 71: Line 72: private ValidationResult testIdNullity() { Line 73: if (macPool.getId() == null) { > This should be done by javax.validation on the MacPool itself Done Line 74: return new ValidationResult(VdcBllMessages.ACTION_TYPE_FAILED_MAC_POOL_DOES_NOT_EXIST); Line 75: } else { Line 76: return null; Line 77: } Line 79: Line 80: private boolean macPoolNameUnique() { Line 81: final List<MacPool> macPools = getMacPoolDao().getAll(); Line 82: for (MacPool pool : macPools) { Line 83: if ((pool.getId() != null && !pool.getId().equals(macPool.getId())) && > You could use Objects.equals if you're concerned about null I do believe it's written like this to have one method, which can check name uniqueness both for updating record usecase and persisting new record usecase. And check for unique name is done prior to persisting record, thus ID is not set. When you've mentioned this, I've reconsidered, whether it wouldn't be better to set ID prior to name checking and then refused it, since name uniqueness of new record has nothing to do with when/how id is generated. So I just used method Objects.equals which I wasn't familiar with. Line 84: pool.getName().equals(macPool.getName())) { Line 85: return false; Line 86: } Line 87: } http://gerrit.ovirt.org/#/c/28705/27/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/UpdateMacPoolValidator.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/UpdateMacPoolValidator.java: Line 2: Line 3: import org.ovirt.engine.core.common.businessentities.MacPool; Line 4: import org.ovirt.engine.core.common.errors.VdcBllMessages; Line 5: Line 6: public class UpdateMacPoolValidator { > Should inherit MacPoolValidator and probably be an inner class of UpdateMac I cannot see neither reason why to inherit, nor way how to do it(provided that two argument constructor stays). If I'd do so, methods on parent would validate what? Old or new MacPool? Would that always work? I.e. is it granted that there won't be usecase demanding this check to be done on other instance. (I cannot make this old/new decision and cannot grant that potential discrepancy, so this classes does not form OO hierarchy). If I'd go with your proposal to remove 'newMacPool' parameter, and introduce hierarchy, I still wouldn't be able to do all validations in UpdateMacPoolCommand via UpdateMacPoolValidator; I'd just save one line at cost of questionable hierarchy/design. I agree that validation in UpdateMacPoolCommand is ugly, but this isn't better. ——— I've moved this class to be inner class of UpdateMacPoolCommand. Line 7: private final MacPool macPoolFromDb; Line 8: private final MacPool newMacPool; Line 9: Line 10: public UpdateMacPoolValidator(MacPool macPoolFromDb, MacPool newMacPool) { Line 15: this.macPoolFromDb = macPoolFromDb; Line 16: this.newMacPool = newMacPool; Line 17: } Line 18: Line 19: public ValidationResult defaultFlagIsNotChanged() { > You could receive the new pool in parameters, sparing the need to save in f is this suggestion or request? I could, but at cost of binding these two classes together(spaghetti), turning this class to non-static(performance) and loosing code clarity(readability). This brings more detrimental effects than any beneficial one (is there any?) Line 20: final boolean failed = macPoolFromDb.isDefaultPool() != newMacPool.isDefaultPool(); Line 21: return ValidationResult.failWith(VdcBllMessages.ACTION_TYPE_FAILED_CHANGING_DEFAULT_MAC_POOL_IS_NOT_SUPPORTED). Line 22: when(failed); Line 23: } Line 16: this.newMacPool = newMacPool; Line 17: } Line 18: Line 19: public ValidationResult defaultFlagIsNotChanged() { Line 20: final boolean failed = macPoolFromDb.isDefaultPool() != newMacPool.isDefaultPool(); > Probably a better name would be "defaultChanged", I would even inline it.. Done Line 21: return ValidationResult.failWith(VdcBllMessages.ACTION_TYPE_FAILED_CHANGING_DEFAULT_MAC_POOL_IS_NOT_SUPPORTED). Line 22: when(failed); Line 23: } Line 18: Line 19: public ValidationResult defaultFlagIsNotChanged() { Line 20: final boolean failed = macPoolFromDb.isDefaultPool() != newMacPool.isDefaultPool(); Line 21: return ValidationResult.failWith(VdcBllMessages.ACTION_TYPE_FAILED_CHANGING_DEFAULT_MAC_POOL_IS_NOT_SUPPORTED). Line 22: when(failed); > The operator (dot) should be in the start of this line, making it easier to Done Line 23: } http://gerrit.ovirt.org/#/c/28705/27/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/MacPoolValidatorTest.java File backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/MacPoolValidatorTest.java: Line 19: Line 20: public class MacPoolValidatorTest { Line 21: Line 22: private MacPool macPool; Line 23: private MacPoolDao macPoolDaoMock; > You should use @Mock Done Line 24: Line 25: @Before Line 26: public void setUp() throws Exception { Line 27: macPool = new MacPool(); -- 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: 27 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
