Martin Mucha has posted comments on this change. Change subject: core: few fixes in MacAddressRangeUtils.java ......................................................................
Patch Set 18: (6 comments) answers 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? Done 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.. what are you asking about? Whether this method should not be defined on caller class instead? If that's the question, then I do believe that NO ONE searches through code base whether there's somewhere code he/she needs, and even if he/she will do that, when CR's are this hard, he/she wouldn't attempt any refactoring. So actually not placing it 'right' during first push means potential code duplicates in future. So answer: although this is method has only one usage, it's placed properly; it lies next to method used to translate sole macAddress forming nice twins. 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? Done 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 i what's the point? It was used during this long refactoring and because of style changing RFE and bringing it to life this disappeared. So now we know that in the end of this patch series this method will disappear. We will merge (or this was at least the plan) all this topic. So what benefit we got via fiddling with method we know is about to be removed? Don't get me wrong, I can move it to another commit/class/do whatever with it. I don't just get it what's the point. 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 that drops that trimming of whitespace. but done, replaced. 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 Done 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 <mmu...@redhat.com> Gerrit-Reviewer: Martin Mucha <mmu...@redhat.com> Gerrit-Reviewer: Mike Kolesnik <mkole...@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
