Mike Kolesnik has posted comments on this change. Change subject: core: util for removing overlaps in ranges ......................................................................
Patch Set 6: (9 comments) http://gerrit.ovirt.org/#/c/26403/6/backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/RangesWithoutOverlaps.java File backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/RangesWithoutOverlaps.java: Line 8: import org.ovirt.engine.core.common.utils.Pair; Line 9: Line 10: public class RangesWithoutOverlaps { Line 11: Line 12: private LinkedList<Pair<Long, Long>> result = new LinkedList<>(); What is the importance of holding a reference to a LinkedList and not just a List? Line 13: Line 14: Line 15: public static List<Pair<Long, Long>> removePotentialOverlaps(LinkedList<Pair<Long, Long>> pairs) { Line 16: if (pairs.isEmpty()) { Line 12: private LinkedList<Pair<Long, Long>> result = new LinkedList<>(); Line 13: Line 14: Line 15: public static List<Pair<Long, Long>> removePotentialOverlaps(LinkedList<Pair<Long, Long>> pairs) { Line 16: if (pairs.isEmpty()) { Why is it necessary? Even without this, an empty list would be returned.. Line 17: return Collections.emptyList(); Line 18: } Line 19: Line 20: RangesWithoutOverlaps rwo = new RangesWithoutOverlaps(); Line 24: Line 25: return rwo.result; Line 26: } Line 27: Line 28: public void addRange(Pair<Long, Long> rangeBeingAdded) { rangeBeingAdded is a bit overkill, just 'range' would be suffice IMHO Line 29: final long addedLeft = rangeBeingAdded.getFirst(); Line 30: final long addedRight = rangeBeingAdded.getSecond(); Line 31: Line 32: addRange(addedLeft, addedRight); Line 31: Line 32: addRange(addedLeft, addedRight); Line 33: } Line 34: Line 35: public void addRange(long addedLeft, long addedRight) { I think start/end or min/max would be more descriptive names Line 36: if (addedLeft > addedRight) { Line 37: throw new IllegalArgumentException("badly defined range"); Line 38: } Line 39: http://gerrit.ovirt.org/#/c/26403/6/backend/manager/modules/utils/src/test/java/org/ovirt/engine/core/utils/RangesWithoutOverlapsTest.java File backend/manager/modules/utils/src/test/java/org/ovirt/engine/core/utils/RangesWithoutOverlapsTest.java: Line 19: Line 20: private Pair<Long, Long>[] inputRanges; Line 21: private final Pair<Long, Long>[] expectedRanges; Line 22: private final Class<? extends Exception> expectedExceptionClass; Line 23: private final String expectedErrorMessage; I wouldn't expect a specific message.. What if someone changes the text and neglects the test? I don't think it renders the change as bad, just because he doesn't agree with the message the test expects. Line 24: Line 25: @Parameterized.Parameters Line 26: public static List<Object[]> parameters() { Line 27: return Arrays.asList(new Object[][]{ Line 57: } Line 58: Line 59: @Test Line 60: public void test() throws Exception { Line 61: LinkedList<Pair<Long, Long>> pairs = new LinkedList<>(Arrays.asList(inputRanges)); Why not do this where you build the parameters? Line 62: Line 63: if (expectedExceptionClass != null) { Line 64: this.expectedException.expect(expectedExceptionClass); Line 65: this.expectedException.expectMessage(expectedErrorMessage); Line 66: } Line 67: Line 68: List<Pair<Long, Long>> result = RangesWithoutOverlaps.removePotentialOverlaps(pairs); Line 69: Line 70: List<Pair<Long, Long>> expectedPairs = Arrays.asList(expectedRanges); Why not do this where you build the parameters? Line 71: Line 72: System.out.println("testing -- Input: "+Arrays.toString(pairs.toArray())); Line 73: Assert.assertArrayEquals("Input: " + Arrays.toString(pairs.toArray()) + "\nActual: " + Arrays.toString(result.toArray()) + "\n" + "Expected: " + Arrays.toString(expectedPairs.toArray()) + "\n\n", result.toArray(), expectedPairs.toArray()); Line 74: } Line 68: List<Pair<Long, Long>> result = RangesWithoutOverlaps.removePotentialOverlaps(pairs); Line 69: Line 70: List<Pair<Long, Long>> expectedPairs = Arrays.asList(expectedRanges); Line 71: Line 72: System.out.println("testing -- Input: "+Arrays.toString(pairs.toArray())); Why do you need to print to stdout? Line 73: Assert.assertArrayEquals("Input: " + Arrays.toString(pairs.toArray()) + "\nActual: " + Arrays.toString(result.toArray()) + "\n" + "Expected: " + Arrays.toString(expectedPairs.toArray()) + "\n\n", result.toArray(), expectedPairs.toArray()); Line 74: } Line 75: Line 76: @Ignore("just add few more permutations of input by shuffling parameters.") Line 69: Line 70: List<Pair<Long, Long>> expectedPairs = Arrays.asList(expectedRanges); Line 71: Line 72: System.out.println("testing -- Input: "+Arrays.toString(pairs.toArray())); Line 73: Assert.assertArrayEquals("Input: " + Arrays.toString(pairs.toArray()) + "\nActual: " + Arrays.toString(result.toArray()) + "\n" + "Expected: " + Arrays.toString(expectedPairs.toArray()) + "\n\n", result.toArray(), expectedPairs.toArray()); Expected should be first and actual second. Line 74: } Line 75: Line 76: @Ignore("just add few more permutations of input by shuffling parameters.") Line 77: @Test -- To view, visit http://gerrit.ovirt.org/26403 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Id7dbacd11b610a5885d574356a695c6e879dcdbc 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: Moti Asayag <[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
