Martin Mucha has posted comments on this change. Change subject: core: util for removing overlaps in ranges ......................................................................
Patch Set 15: (15 comments) I accepted new code for overlaps util, which renders most answers irrelevant, since after that change, chain of successive fixes is now possible. But I posted the answers anyway. 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. I think this is a classic example of methods so simple and obvious that one should not comment them, but OK, added. DONE. 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. ok, that static method is wrong, I do agree. Will drop it and fix OO design of this class. 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? Done 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 abov Done 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. ok, this is readable. I can throw away mine implementation with this as a replacement. 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. Done 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.. It's matter of taste. There are actually recommendations just to stick to List as a default if possible to make code more readable. But you're right, that to write more versatile code I could use Collection as a parameter in DisjointRanges implementation — argumentation is: for cases when somebody will want to pass Set lets say. Fact that order of List is not used is not important. 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 obli Need not. While you're right with Collection as input parameter in implementation being tested, we do know, what this class produces as an output. So this test 'whitetests' that created collection. It's fine. New collection instance is created there and filled with new data, it's valid to return specific instance even if you're not forced to do so. Returning Collection makes it no better, only potentially more problematic when dealing with surrounding code, since List is most common collection type. Note: List does not necessarily mean, that order of elements in them are important, it only says, that you can use index to access elements. Arrays also is structure with order and no one would think about 'fixing' that. EDIT: after change in implementation not returning ranges in order I've changed it to Collections as requested(not be surprised that Collection type(which is not very common to use) is now lurking through mine code). 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? Done 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 Done 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.. Done 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.. Done 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 implem tests need to perform always the same if possible. Randomization is no good. 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"? tell me, please, why is this so important to you? I know, that 'this' can be removed. I asked others about their opinion and for example found a guy, which would actually like 'this' to be specified *everytime* when accessing field. Actually I don't care of of this modifier: "you want it, use it; you don't want to use it, don't use it.". What objective criteria are you following that leads you you to the thinking "this should be removed/discussed?" 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 i this is a grey area(I hope you'll notice and like this pun). You'd expect more blackbox test, I wrote more whitebox one. There's nothing wrong about it; it's more like a strawman fallacy. You expect it to be blackbox test, then complaining about its whiteness. It's not blackbox test. I do like that the implementation returns List, I think it's just fine. Implementation guaranties, that intervals gets sorted as a side effect, which is also nice. EDIT: I've just changed implementation as you requested, so no ordered side-effect, i.e. I'm comparing lists to have same content. I'll also change the return type to Collection for you(not be surprised that Collection type(which is not very common to use) is now lurking through mine code). 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
