Mike Kolesnik has posted comments on this change. Change subject: core: few fixes in MacAddressRangeUtils.java ......................................................................
Patch Set 18: (6 comments) http://gerrit.ovirt.org/#/c/26404/18/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 10: import org.ovirt.engine.core.utils.log.LogFactory; Line 11: Line 12: public class MacAddressRangeUtils { Line 13: Line 14: private static final Log LOGGER = LogFactory.getLog(MacAddressRangeUtils.class); Why do you need unused member? Line 15: private static final int HEX_RADIX = 16; Line 16: Line 17: public static final long MAC_ADDRESS_MULTICAST_BIT = 0x010000000000L; Line 18: Line 47: Line 48: return macAddresses; Line 49: } Line 50: Line 51: public static List<String> macAddressesToStrings(List<Long> macAddresses) { Why is this method necessary here? There is only one place calling it.. Line 52: final ArrayList<String> result = new ArrayList<>(macAddresses.size()); Line 53: Line 54: for (Long macAddress : macAddresses) { Line 55: result.add(macToString(macAddress)); Line 48: return macAddresses; Line 49: } Line 50: Line 51: public static List<String> macAddressesToStrings(List<Long> macAddresses) { Line 52: final ArrayList<String> result = new ArrayList<>(macAddresses.size()); Why hold a reference to ArrayList? Line 53: Line 54: for (Long macAddress : macAddresses) { Line 55: result.add(macToString(macAddress)); Line 56: } Line 57: Line 58: return result; Line 59: } Line 60: Line 61: public static List<Pair<Long, Long>> rangesTextualDefinitionToRangeBoundaries(String ranges) { Why is this necessary here? Who would be calling it except one place (and in the end of the patch series, no one)? Line 62: if (ranges == null || ranges.trim().isEmpty()) { Line 63: return Collections.emptyList(); Line 64: } Line 65: Line 58: return result; Line 59: } Line 60: Line 61: public static List<Pair<Long, Long>> rangesTextualDefinitionToRangeBoundaries(String ranges) { Line 62: if (ranges == null || ranges.trim().isEmpty()) { You could more simply check with StringUtils.isEmpty Line 63: return Collections.emptyList(); Line 64: } Line 65: Line 66: String[] rangesArray = ranges.split("[,]", -1); http://gerrit.ovirt.org/#/c/26404/18/backend/manager/modules/utils/src/test/java/org/ovirt/engine/core/utils/MacAddressRangeUtilsTest.java File backend/manager/modules/utils/src/test/java/org/ovirt/engine/core/utils/MacAddressRangeUtilsTest.java: Line 19: public void testMacToLong() throws Exception { Line 20: final String macString = "00:1A:4A:01:00:00"; Line 21: final long macLong = MacAddressRangeUtils.macToLong(macString); Line 22: Line 23: Assert.assertThat(MacAddressRangeUtils.macToString(macLong), CoreMatchers.is(macString)); You should use the actual number, otherwise you're checking this method as well. It would make sense to extract both to constants (for sake of the testMacToString also) Line 24: } -- 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: 18 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
