Mike Kolesnik has posted comments on this change. Change subject: core: util for removing overlaps in ranges ......................................................................
Patch Set 15: Code-Review-1 (15 comments) Please also run the formatter on the code http://gerrit.ovirt.org/#/c/26403/15/backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/DisjointRanges.java File backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/DisjointRanges.java: Line 6: import java.util.ListIterator; Line 7: Line 8: import org.ovirt.engine.core.common.utils.Pair; Line 9: Line 10: public class DisjointRanges { You should add some javadoc at least for the public methods. Line 11: Line 12: private List<Pair<Long, Long>> disjointRanges = new LinkedList<>(); Line 13: Line 14: Line 8: import org.ovirt.engine.core.common.utils.Pair; Line 9: Line 10: public class DisjointRanges { Line 11: Line 12: private List<Pair<Long, Long>> disjointRanges = new LinkedList<>(); Not sure why keeping state is necessary for a static computation. I believe you won't be keeping copies of this class around, so defining state seems unnecessary. Line 13: Line 14: Line 15: public static List<Pair<Long, Long>> removePotentialOverlaps(List<Pair<Long, Long>> pairs) { Line 16: DisjointRanges rwo = new DisjointRanges(); Line 12: private List<Pair<Long, Long>> disjointRanges = new LinkedList<>(); Line 13: Line 14: Line 15: public static List<Pair<Long, Long>> removePotentialOverlaps(List<Pair<Long, Long>> pairs) { Line 16: DisjointRanges rwo = new DisjointRanges(); What does rwo mean? Line 17: for (Pair<Long, Long> pair : pairs) { Line 18: rwo.addRange(pair); Line 19: } Line 20: Line 20: Line 21: return rwo.disjointRanges; Line 22: } Line 23: Line 24: public void addRange(Pair<Long, Long> range) { Why is this method necessary? Anyone using it except the static method above? Seems like unnecessary lines to me when the static method can just send the values of first & second by itself Line 25: final long addedLeft = range.getFirst(); Line 26: final long addedRight = range.getSecond(); Line 27: Line 28: addRange(addedLeft, addedRight); Line 27: Line 28: addRange(addedLeft, addedRight); Line 29: } Line 30: Line 31: public void addRange(long addedLeft, long addedRight) { Using apache commons` LongRange this function can be simplified greatly. Please see http://hastebin.com/tofetoroxi You'll find that it satisfies the tests (once they're fixed). Line 32: if (addedLeft > addedRight) { Line 33: throw new IllegalArgumentException("badly defined range"); Line 34: } Line 35: http://gerrit.ovirt.org/#/c/26403/15/backend/manager/modules/utils/src/test/java/org/ovirt/engine/core/utils/DisjointRangesTest.java File backend/manager/modules/utils/src/test/java/org/ovirt/engine/core/utils/DisjointRangesTest.java: Line 3: import java.util.Arrays; Line 4: import java.util.Collections; Line 5: import java.util.List; Line 6: Line 7: import org.junit.Assert; You should import assert methods statically. Line 8: import org.junit.Rule; Line 9: import org.junit.Test; Line 10: import org.junit.rules.ExpectedException; Line 11: import org.junit.runner.RunWith; Line 14: Line 15: @RunWith(Parameterized.class) Line 16: public class DisjointRangesTest { Line 17: Line 18: private List<Pair<Long, Long>> inputRanges; This should be a Collection of ranges, since order shouldn't matter.. Line 19: private final List<Pair<Long, Long>> expectedRanges; Line 20: private final Class<? extends Exception> expectedExceptionClass; Line 21: private final String expectedErrorMessage; Line 22: Line 15: @RunWith(Parameterized.class) Line 16: public class DisjointRangesTest { Line 17: Line 18: private List<Pair<Long, Long>> inputRanges; Line 19: private final List<Pair<Long, Long>> expectedRanges; This should also be a Collection since there's no need for this API to obligate to any type of ordering. Line 20: private final Class<? extends Exception> expectedExceptionClass; Line 21: private final String expectedErrorMessage; Line 22: Line 23: @Parameterized.Parameters Line 30: { Arrays.asList( pair( 1, 3 ), pair( 2, 5 ) ), Arrays.asList( pair( 1, 5 ) ), null, null }, Line 31: { Arrays.asList( pair( 1, 3 ), pair( 5, 7 ), pair( 6, 7) ), Arrays.asList( pair( 1, 3 ), pair(5, 7) ), null, null }, Line 32: { Arrays.asList( pair( 1, 2 ), pair( 5, 7 ), pair( 4, 7) ), Arrays.asList( pair( 1, 2 ), pair(4, 7) ), null, null }, Line 33: { Arrays.asList( pair( 1, 3 ), pair( 5, 7 ), pair( 4, 7) ), Arrays.asList( pair( 1, 3), pair(4, 7 ) ), null, null }, Line 34: { Arrays.asList( pair( 1, 3 ), pair( 5, 6 ), pair( 9, 11), pair(3, 11) ), Arrays.asList( pair (1, 3), pair( 3, 11 ) ), null, null }, Not sure how 1-3 and 3-11 aren't overlapping? Line 35: { Arrays.asList( pair( 1, 3 ), pair( 5, 6 ), pair( 9, 11), pair(3, 12) ), Arrays.asList( pair( 1, 3), pair(3, 12 ) ), null, null }, Line 36: { Arrays.asList( pair( 1, 3 ), pair( 5, 6 ), pair( 9, 11), pair(3, 8) ), Arrays.asList( pair( 1, 3), pair(3, 8), pair(9, 11) ), null, null }, Line 37: { Arrays.asList( pair( 1, 3 ), pair( 5, 6 ), pair( 9, 11), pair(-1, 8) ), Arrays.asList( pair( -1, 8), pair(9, 11 ) ), null, null } Line 38: Line 31: { Arrays.asList( pair( 1, 3 ), pair( 5, 7 ), pair( 6, 7) ), Arrays.asList( pair( 1, 3 ), pair(5, 7) ), null, null }, Line 32: { Arrays.asList( pair( 1, 2 ), pair( 5, 7 ), pair( 4, 7) ), Arrays.asList( pair( 1, 2 ), pair(4, 7) ), null, null }, Line 33: { Arrays.asList( pair( 1, 3 ), pair( 5, 7 ), pair( 4, 7) ), Arrays.asList( pair( 1, 3), pair(4, 7 ) ), null, null }, Line 34: { Arrays.asList( pair( 1, 3 ), pair( 5, 6 ), pair( 9, 11), pair(3, 11) ), Arrays.asList( pair (1, 3), pair( 3, 11 ) ), null, null }, Line 35: { Arrays.asList( pair( 1, 3 ), pair( 5, 6 ), pair( 9, 11), pair(3, 12) ), Arrays.asList( pair( 1, 3), pair(3, 12 ) ), null, null }, Same question for 1-3 and 3-12 Line 36: { Arrays.asList( pair( 1, 3 ), pair( 5, 6 ), pair( 9, 11), pair(3, 8) ), Arrays.asList( pair( 1, 3), pair(3, 8), pair(9, 11) ), null, null }, Line 37: { Arrays.asList( pair( 1, 3 ), pair( 5, 6 ), pair( 9, 11), pair(-1, 8) ), Arrays.asList( pair( -1, 8), pair(9, 11 ) ), null, null } Line 38: Line 39: }); Line 32: { Arrays.asList( pair( 1, 2 ), pair( 5, 7 ), pair( 4, 7) ), Arrays.asList( pair( 1, 2 ), pair(4, 7) ), null, null }, Line 33: { Arrays.asList( pair( 1, 3 ), pair( 5, 7 ), pair( 4, 7) ), Arrays.asList( pair( 1, 3), pair(4, 7 ) ), null, null }, Line 34: { Arrays.asList( pair( 1, 3 ), pair( 5, 6 ), pair( 9, 11), pair(3, 11) ), Arrays.asList( pair (1, 3), pair( 3, 11 ) ), null, null }, Line 35: { Arrays.asList( pair( 1, 3 ), pair( 5, 6 ), pair( 9, 11), pair(3, 12) ), Arrays.asList( pair( 1, 3), pair(3, 12 ) ), null, null }, Line 36: { Arrays.asList( pair( 1, 3 ), pair( 5, 6 ), pair( 9, 11), pair(3, 8) ), Arrays.asList( pair( 1, 3), pair(3, 8), pair(9, 11) ), null, null }, Again, 1-3 and 3-8 overlap.. Line 37: { Arrays.asList( pair( 1, 3 ), pair( 5, 6 ), pair( 9, 11), pair(-1, 8) ), Arrays.asList( pair( -1, 8), pair(9, 11 ) ), null, null } Line 38: Line 39: }); Line 40: } Line 44: } Line 45: Line 46: Line 47: @Rule Line 48: public ExpectedException expectedException = ExpectedException.none(); Not sure why you defined this field in the middle of the class.. Line 49: Line 50: public DisjointRangesTest(List<Pair<Long, Long>> inputRanges, Line 51: List<Pair<Long, Long>> expectedRanges, Line 52: Class<? extends Exception> expectedExceptionClass, Line 58: this.expectedErrorMessage = expectedErrorMessage; Line 59: } Line 60: Line 61: @Test Line 62: public void test() throws Exception { You should randomize the source collection order, as you're using an implementation of list, to negate any effects that ordering could have. Line 63: Line 64: if (expectedExceptionClass != null) { Line 65: this.expectedException.expect(expectedExceptionClass); Line 66: this.expectedException.expectMessage(expectedErrorMessage); Line 62: public void test() throws Exception { Line 63: Line 64: if (expectedExceptionClass != null) { Line 65: this.expectedException.expect(expectedExceptionClass); Line 66: this.expectedException.expectMessage(expectedErrorMessage); Why do you call with "this"? Line 67: } Line 68: Line 69: List<Pair<Long, Long>> result = DisjointRanges.removePotentialOverlaps(inputRanges); Line 70: Assert.assertArrayEquals("Input: " + Arrays.toString(inputRanges.toArray()) + "\nActual: " + Arrays.toString(result.toArray()) + "\n" + "Expected: " + Arrays.toString(expectedRanges.toArray()) + "\n\n", expectedRanges.toArray(), result.toArray()); Line 66: this.expectedException.expectMessage(expectedErrorMessage); Line 67: } Line 68: Line 69: List<Pair<Long, Long>> result = DisjointRanges.removePotentialOverlaps(inputRanges); Line 70: Assert.assertArrayEquals("Input: " + Arrays.toString(inputRanges.toArray()) + "\nActual: " + Arrays.toString(result.toArray()) + "\n" + "Expected: " + Arrays.toString(expectedRanges.toArray()) + "\n\n", expectedRanges.toArray(), result.toArray()); This is a bad assertion since it relies on the order of the arrays, which is irrelevant as far as I can tell from the defined API. You should instead test that the two collections have the exact same items for example with CollectionUtils.isEqualCollection Line 71: } -- 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: 15 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: 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
