Martin Mucha has posted comments on this change. Change subject: core: few fixes in MacAddressRangeUtils.java ......................................................................
Patch Set 6: (11 comments) answers 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 8: import org.ovirt.engine.core.common.utils.Pair; Line 9: import org.ovirt.engine.core.utils.log.Log; Line 10: import org.ovirt.engine.core.utils.log.LogFactory; Line 11: Line 12: public class MacAddressRangeUtils { > Would be good to see some unit test here yes, this is just a refactor of one great untested code. After I left working on pools, code coverage went from 0% to approx 50%. I'll do my best to improve this, but that's outside of refactor scope. Line 13: Line 14: private static final Log LOGGER = LogFactory.getLog(MacAddressRangeUtils.class); Line 15: private static final int HEX_RADIX = 16; Line 16: 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? to preserve same behavior as there already was. This is just moved method (although somewhat improved). 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? not to have to write boilerplate loops? If you ment "why is not called" — it will called in following commits. renamed. 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 Done 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.. it's refactor, not rewrite ~~ not mine code, not mine decision. This was in your code base, I just moved it. I cannot change behavior, and your behavior is: "be lenient when some part of ranges definition is malformed". 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'? Done 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.. Done 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. Done 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).. original code catches Exception. If former code did not fail, neither this can. 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? former class was lenient, I cannot change it. 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? to be able to catch it. 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
