Eliraz Levi has posted comments on this change. Change subject: tools: CidrValidation Utils ......................................................................
Patch Set 1: (12 comments) http://gerrit.ovirt.org/#/c/32539/1/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/utils/ValidationUtils.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/utils/ValidationUtils.java: Line 23: public static final String NO_SPECIAL_CHARACTERS_WITH_DOT = "[0-9a-zA-Z-_\\.]+"; Line 24: public static final String NO_TRIMMING_WHITE_SPACES_PATTERN = "^$|\\S.*\\S"; Line 25: public static final String IP_PATTERN = Line 26: "^\\b((25[0-5]|2[0-4]\\d|[01]\\d\\d|\\d?\\d)\\.){3}(25[0-5]|2[0-4]\\d|[01]\\d\\d|\\d?\\d)\\b$|^$"; Line 27: //TODO elevi consider name - could be miss leading > why miss leading ? Done Line 28: public static final String CIDR_FORMAT_PATTERN = Line 29: "\\b((25[0-5]|2[0-4]\\d|[01]\\d\\d|\\d?\\d)\\.){3}(25[0-5]|2[0-4]\\d|[01]\\d\\d|\\d?\\d)(?:/(?:3[0-2]|[12]?[0-9]))$"; Line 30: public static final String ISO_SUFFIX = ".iso"; Line 31: public static final String ISO_SUFFIX_PATTERN = "^$|^.+\\.iso$"; http://gerrit.ovirt.org/#/c/32539/1/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/validation/CidrValidator.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/validation/CidrValidator.java: Line 2: Line 3: import org.ovirt.engine.core.common.utils.ValidationUtils; Line 4: Line 5: public class CidrValidator { Line 6: private static String getCidrRegex() { > why do you need this method instead of accessing ValidationUtils.CIDR_FORMA Done Line 7: return ValidationUtils.CIDR_FORMAT_PATTERN; Line 8: } Line 9: Line 10: /*** Line 2: Line 3: import org.ovirt.engine.core.common.utils.ValidationUtils; Line 4: Line 5: public class CidrValidator { Line 6: private static String getCidrRegex() { > why do you need this method instead of accessing ValidationUtils.CIDR_FORMA Done Line 7: return ValidationUtils.CIDR_FORMAT_PATTERN; Line 8: } Line 9: Line 10: /*** Line 6: private static String getCidrRegex() { Line 7: return ValidationUtils.CIDR_FORMAT_PATTERN; Line 8: } Line 9: Line 10: /*** > javadoc can be formatted to ease the reading, for example: Done Line 11: * check if CIDR is in correct format - x.x.x.x/y where: x belongs to [0,255] y belongs to [0,32] both inclusive Line 12: * note! the function is not validating that ip and mask match to a network address, please see @see Line 13: * {@link CidrValidator#isCidrFormatValid(String)} Line 14: * @param cidr Line 6: private static String getCidrRegex() { Line 7: return ValidationUtils.CIDR_FORMAT_PATTERN; Line 8: } Line 9: Line 10: /*** > javadoc can be formatted to ease the reading, for example: Done Line 11: * check if CIDR is in correct format - x.x.x.x/y where: x belongs to [0,255] y belongs to [0,32] both inclusive Line 12: * note! the function is not validating that ip and mask match to a network address, please see @see Line 13: * {@link CidrValidator#isCidrFormatValid(String)} Line 14: * @param cidr Line 14: * @param cidr Line 15: * @return true if correct format, false otherwise. Line 16: */ Line 17: public static boolean isCidrFormatValid(String cidr) { Line 18: return (cidr == null || !cidr.matches(getCidrRegex())) ? > doesn't it mean that a user which sends an empty cidr (i.e. wasn't supplied yes, is it wrong? Line 19: false : true; Line 20: } Line 21: Line 22: /*** Line 25: * - in correct format, please use the following function first: @see Line 26: * {@link CidrValidator#isCidrFormatValid(String)} Line 27: * @return true if valid CIDR ,false otherwise Line 28: */ Line 29: public static boolean isCidrValidNetworkAddress(String cidr) { > maybe the name should be isCidrNetworkAddressValid ? Done Line 30: String ipAsString = getIpFromCidr(cidr); Line 31: int ipAsInteger = covnertIpToInt(ipAsString); Line 32: int mask = getMaskFromCidr(cidr); Line 33: return isNetworkAddress(ipAsInteger, mask); Line 34: } Line 35: Line 36: private static int getMaskFromCidr(String cidr) { Line 37: String[] temp = cidr.split("/"); Line 38: return Integer.parseInt(temp[1]); > why there is needs to parse the string twice ? i need to extract ip and mask from the cidr. each time for different purpose. alternatives: 1. create class with two members - ip and mask (my opinion: overkill) 2. add to the parse function index param which will identify which part of cidr is needed. (my opinion: code will become less clear, can use enum but then again, i think it's overkil) 3. give array of two strings as input Line 39: } Line 40: Line 41: private static String getIpFromCidr(String cidr) { Line 42: String[] temp = cidr.split("/"); Line 50: for (int index = 3; index > -1; index--) { Line 51: temp = Integer.parseInt(subAdd[3 - index]); Line 52: temp <<= (index * 8); Line 53: output |= temp; Line 54: } > please add a space line after a closing curly bracket Done Line 55: return output; Line 56: } Line 57: Line 58: private static boolean isNetworkAddress(int ip, int mask) { Line 58: private static boolean isNetworkAddress(int ip, int mask) { Line 59: int temp = 1; Line 60: int check = 0; Line 61: for (int i = 0; i < mask; i++) { Line 62: check = temp & ip; > wouldn't you can avoid 'check' variable here ? can do (temp & ip != 0) it is a bit wise, so i thought the code will be clearer this way. if you wish , can do as proposed Line 63: if (check != 0) { Line 64: return false; Line 65: } Line 66: temp <<= 1; Line 61: for (int i = 0; i < mask; i++) { Line 62: check = temp & ip; Line 63: if (check != 0) { Line 64: return false; Line 65: } > same about space line Done Line 66: temp <<= 1; Line 67: } Line 68: return true; Line 69: } http://gerrit.ovirt.org/#/c/32539/1/backend/manager/modules/common/src/test/java/org/ovirt/engine/core/common/validation/CidrValidatorTest.java File backend/manager/modules/common/src/test/java/org/ovirt/engine/core/common/validation/CidrValidatorTest.java: Line 21: } Line 22: Line 23: @Test Line 24: public void checkCidrValidation() { Line 25: boolean result = CidrValidator.isCidrFormatValid(cidr); > why not to separate into two tests ? please see replay in CidrAnnotationTest ( " can be implemented with a cost of not using @Parameterized.Parameters. explanation: say we have input 1.1.1.1/1 for isCidrFormatValid validation, it will be saved in Param's collection as ("1.1.1.1/1",true) but for isCidrValidNetworkAddress validation it should be saved as ("1.1.1.1/1", false) so i think having one test to check them both is worth the simplicity of using @param. note: isCidrValidNetworkAddress assume for valid format. for tests as well , must first ask isCidrFormatValid before calling to isCidrValidNetworkAddress " ) Line 26: if (result) { Line 27: assertEquals(expectedResult, CidrValidator.isCidrValidNetworkAddress(cidr)); Line 28: } Line 29: else { -- To view, visit http://gerrit.ovirt.org/32539 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ib1277dbc815953926fe1648350cd55cb75e1084a Gerrit-PatchSet: 1 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Eliraz Levi <[email protected]> Gerrit-Reviewer: Alona Kaplan <[email protected]> Gerrit-Reviewer: Eliraz Levi <[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
