aherbert commented on code in PR #335:
URL: https://github.com/apache/commons-math/pull/335#discussion_r4368453606
##########
commons-math-legacy-core/src/main/java/org/apache/commons/math4/legacy/core/IntegerSequence.java:
##########
@@ -287,7 +321,13 @@ public int getCount() {
* {@code true} otherwise.
*/
public boolean canIncrement() {
- return canIncrement(1);
+ if (exhausted) {
Review Comment:
If the count is only ever changed in conjunction with checking for
exhaustion, can this simply return the value of the exhausted flag?
##########
commons-math-legacy-core/src/main/java/org/apache/commons/math4/legacy/core/IntegerSequence.java:
##########
@@ -319,9 +364,20 @@ public void increment(int nTimes) throws
MaxCountExceededException {
throw new NotStrictlyPositiveException(nTimes);
}
- count += nTimes * increment;
+ if (!canIncrement()) {
+ maxCountCallback.trigger(maximalCount);
+ return;
+ }
+
+ final long newCount = count + (long) nTimes * increment;
+ if (increment < 0 ? newCount <= maximalCount : newCount >=
maximalCount) {
+ count = maximalCount;
+ exhausted = true;
+ } else {
+ count = newCount;
+ }
- if (!canIncrement(0)) {
+ if (exhausted || !canIncrement(0)) {
Review Comment:
This should not have to call canIncrement(0) as the exhausted flag has been
set.
##########
commons-math-legacy-core/src/main/java/org/apache/commons/math4/legacy/core/IntegerSequence.java:
##########
@@ -299,7 +339,12 @@ public boolean canIncrement() {
* {@code true} otherwise.
*/
public boolean canIncrement(int nTimes) {
- final int finalCount = count + nTimes * increment;
+ final long finalCount = count + (long) nTimes * increment;
Review Comment:
This logic should be captured once in a method:
```java
private static boolean isExhausted(long newCount) {
if (inclusive) {
return increment < 0 ?
newCount >= maximalCount :
newCount <= maximalCount;
}
return increment < 0 ?
newCount > maximalCount :
newCount < maximalCount;
}
```
The method should be called from `canIncrement(int nTimes)`, and also to set
the exhausted flag within `increment()` and `increment(int)`. If the count is
only ever changed and the `exhausted` flag updated at the same time, then it
can be used within `canIncrement()`. This should not impact any iterator as the
logic is invoked once, just at a different time in the iteration of `hasNext()`
and `next()`.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
