aherbert commented on code in PR #335:
URL: https://github.com/apache/commons-math/pull/335#discussion_r4369991213
##########
commons-math-legacy-core/src/main/java/org/apache/commons/math4/legacy/core/IntegerSequence.java:
##########
@@ -287,7 +321,7 @@ public int getCount() {
* {@code true} otherwise.
*/
public boolean canIncrement() {
- return canIncrement(1);
+ return canIncrement(0);
Review Comment:
This should return `!exhausted`.
##########
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 in a method `isExhausted(long)`
##########
commons-math-legacy-core/src/main/java/org/apache/commons/math4/legacy/core/IntegerSequence.java:
##########
@@ -347,15 +400,24 @@ public void increment() throws MaxCountExceededException {
/** {@inheritDoc} */
@Override
public boolean hasNext() {
- return canIncrement(0);
+ return canIncrement();
}
/** {@inheritDoc} */
@Override
public Integer next() {
- if (canIncrement(0)) {
- final int value = count;
- count += increment;
+ if (canIncrement()) {
+ final int value = (int) count;
+ final long newCount = count + increment;
+ final boolean crossedLimit = increment < 0 ?
+ (inclusive ? newCount < maximalCount : newCount <=
maximalCount) :
+ (inclusive ? newCount > maximalCount : newCount >=
maximalCount);
+ if (inclusive && value == maximalCount || crossedLimit) {
Review Comment:
This should use `isExhausted(newCount)`
##########
commons-math-legacy-core/src/main/java/org/apache/commons/math4/legacy/core/IntegerSequence.java:
##########
@@ -319,9 +361,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) {
Review Comment:
This does not use the inclusive flag. If you refactor the check for
exhausted to a method this should use:
```java
if (isExhausted(newCount)) {
```
This sets the behaviour for the futher calls to the instance (canIncrement,
increment; and next).
##########
commons-math-legacy-core/src/main/java/org/apache/commons/math4/legacy/core/IntegerSequence.java:
##########
@@ -319,9 +361,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 (!canIncrement()) {
Review Comment:
This must trigger if the current call **exceeded** the allowed maximum. This
is the reason for the `canIncrement(0)` logic in the original code.
In the logic above the `exhausted` flag is set to determine the behaviour
for **further** calls. To determine the behaviour for **this** call we must use
the `newCount` flag and check if it went beyond the allowed limit. This is
equivalent to checking if an increment (nTimes - 1) would prevent further
increment. So we can call:
```java
if (isExhausted(newCount - increment)) {
// An increment of (nTimes - 1) would reach the limit ...
```
This is overly complex for the `increment()` logic as it would always be
false for this check when it calls `increment(1)`. So that method can be
updated to have an implementation:
```java
public void increment() {
if (exhausted) {
maxCountCallback.trigger(maximalCount);
return;
}
final long newCount = count + increment;
if (isExhausted(newCount)) {
count = maximalCount;
// Set behaviour for further calls
exhausted = true;
} else {
count = newCount;
}
}
```
--
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]
