Lior Vernia has posted comments on this change. Change subject: webadmin: integrate CidrValidator to frontend ......................................................................
Patch Set 6: (10 comments) http://gerrit.ovirt.org/#/c/32541/6/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/validation/CidrValidation.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/validation/CidrValidation.java: Line 12: } Line 13: Line 14: @Override Line 15: public ValidationResult validate(Object value) { Line 16: assert value instanceof String; 1. Shouldn't you check assert (value == null || value instanceof String)? 2. Please add some text explaining why this assertion was made in case it fails (e.g. "This validation must be applied to a String"). Line 17: String cidr = (String) value; Line 18: ValidationResult result = new ValidationResult(); Line 19: Line 20: if (!CidrValidator.isCidrFormatValid(cidr)) { Line 17: String cidr = (String) value; Line 18: ValidationResult result = new ValidationResult(); Line 19: Line 20: if (!CidrValidator.isCidrFormatValid(cidr)) { Line 21: return failWith(result, getThisFieldMustContainCidrInFormatMsg()); I would not return here. It's cleaner to only set the error and message here... Line 22: } Line 23: Line 24: else if (!CidrValidator.isCidrNetworkAddressValid(cidr)) { Line 25: return failWith(result, getCidrNotNetworkAddress()); Line 21: return failWith(result, getThisFieldMustContainCidrInFormatMsg()); Line 22: } Line 23: Line 24: else if (!CidrValidator.isCidrNetworkAddressValid(cidr)) { Line 25: return failWith(result, getCidrNotNetworkAddress()); ...and here as well... Line 26: } Line 27: else { Line 28: result.setSuccess(true); Line 29: return result; Line 24: else if (!CidrValidator.isCidrNetworkAddressValid(cidr)) { Line 25: return failWith(result, getCidrNotNetworkAddress()); Line 26: } Line 27: else { Line 28: result.setSuccess(true); ...and this isn't needed because ValidationResult is by default a success... Line 29: return result; Line 30: } Line 31: Line 32: } Line 25: return failWith(result, getCidrNotNetworkAddress()); Line 26: } Line 27: else { Line 28: result.setSuccess(true); Line 29: return result; And this needs to be returned anyway, whether there was an error or not. So I would take it out of the conditional clause, and there's no need for the "else" clause at all. Line 30: } Line 31: Line 32: } Line 33: http://gerrit.ovirt.org/#/c/32541/6/frontend/webadmin/modules/uicommonweb/src/test/java/org/ovirt/engine/ui/uicommonweb/validation/CidrValidationTest.java File frontend/webadmin/modules/uicommonweb/src/test/java/org/ovirt/engine/ui/uicommonweb/validation/CidrValidationTest.java: Line 23: private String cidr; Line 24: private boolean validCidrFormatExpectedResult; Line 25: private boolean validNetworkAddressExpectedResult; Line 26: Line 27: public CidrValidationTest(String cidr, boolean validCidrFormatExpectedResult, boolean validNetworkAddressExpectedResult) { I doesn't make sense to have two boolean values when you set the two error messages to be the same (null). You can't distinguish between the two reasons why the validation might have failed - no test could ever pass if it expects true in one and false in the other (that's probably part of the reason why the unit tests fail...). I would use only one boolean to see if the validation passed or not, no matter the reason. Line 28: this.cidr = cidr; Line 29: this.validCidrFormatExpectedResult = validCidrFormatExpectedResult; Line 30: this.validNetworkAddressExpectedResult = validNetworkAddressExpectedResult; Line 31: } Line 86: { "255?23?1?0/8", false, false}, //$NON-NLS-1$ Line 87: { "23\22\22\22\22\\", false, false}, //$NON-NLS-1$ Line 88: { ".................", false, false}, //$NON-NLS-1$ Line 89: { "././././", false, false}, //$NON-NLS-1$ Line 90: { "?/?/?/?/", false, false}, //$NON-NLS-1$ Please add a blank line here, to visibly separate the section. Line 91: // Not A Network address Line 92: { "255.255.255.250/16", true, false}, //$NON-NLS-1$ Line 93: { "255.255.255.255/16", true, false}, //$NON-NLS-1$ Line 94: { "255.255.255.255/1", true, false}, //$NON-NLS-1$ Line 88: { ".................", false, false}, //$NON-NLS-1$ Line 89: { "././././", false, false}, //$NON-NLS-1$ Line 90: { "?/?/?/?/", false, false}, //$NON-NLS-1$ Line 91: // Not A Network address Line 92: { "255.255.255.250/16", true, false}, //$NON-NLS-1$ How are the next ~30 tests different from this one? I would expect a few meaningful examples, not every possible combination. There's no way I could go over all these tests and make sure that they make sense. Line 93: { "255.255.255.255/16", true, false}, //$NON-NLS-1$ Line 94: { "255.255.255.255/1", true, false}, //$NON-NLS-1$ Line 95: { "255.255.255.253/2", true, false}, //$NON-NLS-1$ Line 96: { "255.255.255.247/3", true, false}, //$NON-NLS-1$ Line 127: { "130.0.0.0/31", true, false}, //$NON-NLS-1$ Line 128: { "1.0.0.0/32", true, false}, //$NON-NLS-1$ Line 129: Line 130: // valid CIDR Line 131: { "255.255.255.255/0", true, true}, //$NON-NLS-1$ Same comment here, in my opinion you don't have to test every prefix in the world. Plus every test appears twice for some reason. Line 132: { "255.255.255.254/1", true, true}, //$NON-NLS-1$ Line 133: { "255.255.255.252/2", true, true}, //$NON-NLS-1$ Line 134: { "255.255.255.248/3", true, true}, //$NON-NLS-1$ Line 135: { "255.255.255.240/4", true, true}, //$NON-NLS-1$ http://gerrit.ovirt.org/#/c/32541/6/frontend/webadmin/modules/uicompat/src/main/java/org/ovirt/engine/ui/uicompat/UIConstants.java File frontend/webadmin/modules/uicompat/src/main/java/org/ovirt/engine/ui/uicompat/UIConstants.java: Line 2425: Line 2426: @DefaultStringValue("CPU Profile") Line 2427: String cpuProfileTitle(); Line 2428: Line 2429: @DefaultStringValue("This field must contain CIDR notation of an IP subnet, please ensure that the IP address matches the prefix.\nexample: valid network address: 2.2.0.0/16 \ninvalid: 2.2.0.1/16 ") There's a spacebar character that isn't needed after 16. Line 2430: String CidrNotNetworkAddress(); Line 2431: } -- To view, visit http://gerrit.ovirt.org/32541 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ib09d8943b0265960a35f3dd328dae20247b69fd3 Gerrit-PatchSet: 6 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Eliraz Levi <el...@redhat.com> Gerrit-Reviewer: Eliraz Levi <el...@redhat.com> Gerrit-Reviewer: Lior Vernia <lver...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
