Mike Kolesnik has posted comments on this change. Change subject: core: few fixes in MacAddressRangeUtils.java ......................................................................
Patch Set 6: (10 comments) http://gerrit.ovirt.org/#/c/26404/6/backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/MacAddressRangeUtils.java File backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/MacAddressRangeUtils.java: Line 21: long startNum = macStringToLong(start); Line 22: long endNum = macStringToLong(end); Line 23: Line 24: return innerInitRange(size, startNum, endNum); Line 25: } catch (InvalidMacString e) { Why catch the exception? Line 26: LOGGER.warn(e); Line 27: return Collections.emptyList(); Line 28: } Line 29: } Line 50: // Do a copy to reduce array length (array stored in ArrayList impl.) Line 51: return new ArrayList<>(macAddresses); Line 52: } Line 53: Line 54: public static String[] macAddressToString(long[] macAddress) { Why is this method necessary? Also I'd expect the name to be in plural form.. Line 55: String[] result = new String[macAddress.length]; Line 56: for (int i = 0; i < macAddress.length; i++) { Line 57: result[i] = macAddressToString(macAddress[i]); Line 58: } Line 59: Line 60: return result; Line 61: } Line 62: Line 63: public static List<Pair<Long, Long>> rangesStringToRangeBoundaries(String ranges) { No need to put parameter types in the method name Line 64: if (ranges == null || ranges.trim().isEmpty()) { Line 65: return Collections.emptyList(); Line 66: } Line 67: Line 73: Line 74: if (startEndArray.length == 2) { Line 75: rwo.addRange(macStringToLong(startEndArray[0]), macStringToLong(startEndArray[1])); Line 76: } else { Line 77: LOGGER.errorFormat("Failed to initialize Mac Pool range. Please fix Mac Pool range: {0}", rangesArray[i]); Not sure why would you continue at this stage.. Line 78: } Line 79: } Line 80: Line 81: return rwo.getRanges(); Line 80: Line 81: return rwo.getRanges(); Line 82: } Line 83: Line 84: public static boolean macHasMultiCastBitSet(long mac) { Or perhaps just 'macIsMulticast'? Line 85: return (MAC_ADDRESS_MULTICAST_BIT & mac) != 0; Line 86: } Line 87: Line 88: Line 100: Line 101: return stringBuilder.toString(); Line 102: } Line 103: Line 104: public static long macStringToLong(String mac) { No need to put parameter type in method name.. Line 105: Line 106: try { Line 107: String parsedRangeStart = StringUtils.remove(mac, ':'); Line 108: return Long.parseLong(parsedRangeStart, HEX_RADIX); Line 103: Line 104: public static long macStringToLong(String mac) { Line 105: Line 106: try { Line 107: String parsedRangeStart = StringUtils.remove(mac, ':'); I thnik you forgot to rename the variable. Also it can be inlined entirely.. Line 108: return Long.parseLong(parsedRangeStart, HEX_RADIX); Line 109: } catch (NumberFormatException | NullPointerException e) { Line 110: throw new InvalidMacString(mac, e); Line 111: } Line 105: Line 106: try { Line 107: String parsedRangeStart = StringUtils.remove(mac, ':'); Line 108: return Long.parseLong(parsedRangeStart, HEX_RADIX); Line 109: } catch (NumberFormatException | NullPointerException e) { Not sure why you're catching these exceptions (expecially NPE).. Line 110: throw new InvalidMacString(mac, e); Line 111: } Line 112: } Line 113: Line 117: long startNum = macStringToLong(start); Line 118: long endNum = macStringToLong(end); Line 119: Line 120: result = innerInitRange(1, startNum, endNum); Line 121: } catch (InvalidMacString e) { Why catch exception here? Line 122: LOGGER.warn(e); Line 123: result = Collections.emptyList(); Line 124: } Line 125: return !result.isEmpty(); Line 124: } Line 125: return !result.isEmpty(); Line 126: } Line 127: Line 128: public static class InvalidMacString extends RuntimeException { Why not use just an IllegalArgumentException? Line 129: Line 130: public InvalidMacString(String macString, RuntimeException e) { Line 131: super("Invalid MAC string: " + macString, e); Line 132: } -- To view, visit http://gerrit.ovirt.org/26404 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I098ade5fc3854ad23236abceebb87cc51908c65f Gerrit-PatchSet: 6 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: 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
