You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@commons.apache.org by GitBox <gi...@apache.org> on 2021/11/04 21:29:59 UTC
[GitHub] [commons-collections] ClaudioConsolmagno opened a new pull request #265: [COLLECTIONS-800] Adding partitionBalanced(List,int) method
ClaudioConsolmagno opened a new pull request #265:
URL: https://github.com/apache/commons-collections/pull/265
See [COLLECTIONS-800](https://issues.apache.org/jira/projects/COLLECTIONS/issues/COLLECTIONS-800)
Implementation Notes:
- Given that the `Partition` class is a private static class not used by anything else I think it's ok to just use isBalanced as a boolean instead of, say, an enum or having a separate brand new private class for this new method.
- Added several comments to clarify the logic, let me know if you think that's too much.
- I didn't know how far I could go when refactoring existing tests so I only half changed them for `partition()` method.
- Similarly, existing tests all around seem fairly simple but I thought I should have good coverage and test different scenarios for partitionBalanced() hence added the whole `forEach` test cases (I'm a fan of Spock and data driven testing).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-collections] LarsBodewig commented on a change in pull request #265: [COLLECTIONS-800] Adding partitionBalanced(List,int) method
Posted by GitBox <gi...@apache.org>.
LarsBodewig commented on a change in pull request #265:
URL: https://github.com/apache/commons-collections/pull/265#discussion_r771769961
##########
File path: src/main/java/org/apache/commons/collections4/ListUtils.java
##########
@@ -117,9 +119,25 @@ private Partition(final List<T> list, final int size) {
throw new IndexOutOfBoundsException("Index " + index + " must be less than size " +
listSize);
}
- final int start = index * size;
- final int end = Math.min(start + size, list.size());
- return list.subList(start, end);
+ int start;
+ int currentPartitionSize;
+ if (isBalanced) {
+ // evenly distribute partitions
+ currentPartitionSize = (int) Math.ceil((double) list.size() / (double) listSize);
+ start = index * currentPartitionSize;
+ // remainder of above is the threshold for which indices greater than will have one less element
+ final int threshold = (list.size() % listSize);
+ // when currentPartitionSize is 1 or threshold is 0 there's nothing to balance
+ // when index hasn't crossed the threshold we don't need balancing yet
+ if (currentPartitionSize > 1 && threshold > 0 && index >= threshold) {
+ start -= (index - threshold); // adjust start as partitions before threshold have one less element
+ currentPartitionSize--; // currentPartitionSize is decremented as threshold is crossed
Review comment:
Newbie myself, but I wonder if that could be written more simply:
```java
evenlyDistributedSize = ...
if (index < threshold) { // lists before threshold should contain +1
start = index * (evenlyDistributedSize + 1);
end = start + (evenlyDistributedSize + 1);
} else {
start = index * evenlyDistributedSize + threshold; // offset other starting positions by threshold
end = start + evenlyDistributedSize;
}
```
...or similar (Note: this is untested). The if condition and subtractions look quite complicated in my opinion.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-collections] LarsBodewig commented on a change in pull request #265: [COLLECTIONS-800] Adding partitionBalanced(List,int) method
Posted by GitBox <gi...@apache.org>.
LarsBodewig commented on a change in pull request #265:
URL: https://github.com/apache/commons-collections/pull/265#discussion_r771770406
##########
File path: src/main/java/org/apache/commons/collections4/ListUtils.java
##########
@@ -486,13 +504,52 @@ public static String longestCommonSubsequence(final CharSequence charSequenceA,
* @throws NullPointerException if list is null
* @throws IllegalArgumentException if size is not strictly positive
* @since 4.0
+ * @see ListUtils#partitionBalanced(List, int)
*/
public static <T> List<List<T>> partition(final List<T> list, final int size) {
Objects.requireNonNull(list, "list");
if (size <= 0) {
throw new IllegalArgumentException("Size must be greater than 0");
}
- return new Partition<>(list, size);
+ return new Partition<>(list, size, false);
+ }
+
+ /**
+ * Returns consecutive {@link List#subList(int, int) sublists} of a
+ * list, partitioned in a way to balance entries across all sublists. For example,
+ * partitioning a list containing {@code [a, b, c, d, e]} with a partition
+ * size of 3 yields {@code [[a, b, c], [d, e]]} -- an outer list containing
+ * two inner lists of three and two elements, all in the original order. Partitioning
Review comment:
I see why you would use the example from `ListUtils.partition`, however this could be confusing IMO if you start comparing what method to use and see no difference so far. I would opt for a more clear example here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-collections] coveralls commented on pull request #265: [COLLECTIONS-800] Adding partitionBalanced(List,int) method
Posted by GitBox <gi...@apache.org>.
coveralls commented on pull request #265:
URL: https://github.com/apache/commons-collections/pull/265#issuecomment-969185293
[](https://coveralls.io/builds/44274451)
Coverage increased (+0.002%) to 90.139% when pulling **4557a452168daff3020f477e7b395af86070b37e on ClaudioConsolmagno:COLLECTIONS-800-partition-balanced** into **7ab4e4fe994ed51da0a3aed39ce2500c49a50195 on apache:master**.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org