Mike Kolesnik has posted comments on this change. Change subject: utils: performance improvement ......................................................................
Patch Set 3: (6 comments) http://gerrit.ovirt.org/#/c/25940/3/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: if (parsedRangeEnd == null || parsedRangeStart == null) { Line 22: return Collections.emptyList(); Line 23: } Line 24: Line 25: long startNum = Long.parseLong(parsedRangeStart, HEX_RADIX); These can be inlined since they're used only once Line 26: long endNum = Long.parseLong(parsedRangeEnd, HEX_RADIX); Line 27: return innerInitRange(stopAfter, startNum, endNum); Line 28: } Line 29: Line 31: if (startNum > endNum) { Line 32: return Collections.emptyList(); Line 33: } Line 34: Line 35: //initialize ArrayList for all potential records. (ignore that there need not be that many records. nit: Please write first letter of sentence capitalized, and have a space between the comment slash and the first letter. Line 36: List<String> macAddresses = new ArrayList<>(stopAfter); Line 37: for (long i = startNum; i <= endNum; i++) { Line 38: if ((MAC_ADDRESS_MULTICAST_BIT & i) != 0) { Line 39: continue; Line 32: return Collections.emptyList(); Line 33: } Line 34: Line 35: //initialize ArrayList for all potential records. (ignore that there need not be that many records. Line 36: List<String> macAddresses = new ArrayList<>(stopAfter); You could also determine minimum of stopAfter and endNum - startNum and send this, avoiding need to copy the array at the end. Line 37: for (long i = startNum; i <= endNum; i++) { Line 38: if ((MAC_ADDRESS_MULTICAST_BIT & i) != 0) { Line 39: continue; Line 40: } Line 45: return macAddresses; Line 46: } Line 47: } Line 48: Line 49: //do a copy to reduce array length (array stored in ArrayList impl.) nit: Same as previous remark about comments Line 50: return new ArrayList<>(macAddresses); Line 51: } Line 52: Line 53: private static String macAddressIntToString(long macAddress) { Line 49: //do a copy to reduce array length (array stored in ArrayList impl.) Line 50: return new ArrayList<>(macAddresses); Line 51: } Line 52: Line 53: private static String macAddressIntToString(long macAddress) { I'd just call it macAddressToString since: 1. It's not an int you're expecting. 2. No reason to "hard code" the argument type in the method name.. Line 54: String value = String.format("%012x", macAddress); Line 55: char[] chars = value.toCharArray(); Line 56: Line 57: final StringBuilder stringBuilder = new StringBuilder(); Line 59: for (int pos = 2; pos < value.length(); pos += 2) { Line 60: stringBuilder.append(":") Line 61: .append(chars[pos]) Line 62: .append(chars[pos + 1]); Line 63: } nit: A new line would be welcome here Line 64: return stringBuilder.toString(); Line 65: } Line 66: Line 67: public static boolean isRangeValid(String start, String end) { -- To view, visit http://gerrit.ovirt.org/25940 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I09f87ebd1ea17d09a974ce6ca3e26bd2454a72fc Gerrit-PatchSet: 3 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Martin Mucha <mmu...@redhat.com> Gerrit-Reviewer: Mike Kolesnik <mkole...@redhat.com> Gerrit-Reviewer: Moti Asayag <masa...@redhat.com> Gerrit-Reviewer: Yevgeny Zaspitsky <yzasp...@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
