Martin Mucha has posted comments on this change. Change subject: core: util for removing overlaps in ranges ......................................................................
Patch Set 6: (10 comments) answers. 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 Done 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? you're right, it's not important. It's just simple optimization. No RangesWithoutOverlaps instance is created(nor its content — LinkedList), unmodifiable singleton is returned instead. Do you want to remove it? 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 Done 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 that would drop readability. Since we're iterating over existing ranges, I wanted to be at first sight clear, that this range we're currently adding. It's already hard to understand even with good (for me) 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.. JUnit tests are often considered as contract specification. So this JUnit says, when ... then this specific exception will be thrown(with this specific message). Which is *especially* needed, when throwing such widely used exception like IllegalArgumentException. How can you tell otherwise, that what's being tested acually failed and not some other code? Leaving message would render this test useless. No one "just changes text" and his commit fails "just because of it". It's alright, since he did not run the tests before commit, which he should do anyway. This is indeed very important since JUnit cannot tell the difference from "one just changing messages" and "different part of code than expected has failed". 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? Done 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? Done 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? Done 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. my bad. Done. Line 74: } Line 75: Line 76: @Ignore("just add few more permutations of input by shuffling parameters.") Line 77: @Test Line 74: } Line 75: Line 76: @Ignore("just add few more permutations of input by shuffling parameters.") Line 77: @Test Line 78: public void test_random() throws Exception { > Please remove ignored test method Done Line 79: List<Pair<Long, Long>> inputRangesList = Arrays.asList(this.inputRanges); Line 80: Collections.shuffle(inputRangesList); Line 81: this.inputRanges = inputRangesList.toArray(new Pair[0]); Line 82: 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
