How do I avoid a useless return in a Java method?
I have a situation where the return
statement nested in two for
loops will always be reached, theoretically.
The compiler disagrees and requires a return
statement outside of the for
loop. I'd like to know an elegant way to optimize this method that's beyond my current understanding, and none of my attempted implementations of break seem to work.
Attached is a method from an assignment that generates random integers and returns the iterations cycled through until a second random integer is found, generated within a range passed into the method as an int parameter.
private static int oneRun(int range) {
int[] rInt = new int[range+1]; // Stores the past sequence of ints.
rInt[0] = generator.nextInt(range); // Inital random number.
for (int count = 1; count <= range; count++) { // Run until return.
rInt[count] = generator.nextInt(range); // Add randint to current iteration.
for (int i = 0; i < count; i++) { // Check for past occurence and return if found.
if (rInt[i] == rInt[count]) {
return count;
}
}
}
return 0; // Never reached
}
The compiler's heuristics will never let you omit the last return
. If you're sure it'll never be reached, I'd replace it with a throw
to make the situation clear.
private static int oneRun(int range) {
int[] rInt = new int[range+1]; // Stores the past sequence of ints.
rInt[0] = generator.nextInt(range); // Inital random number.
for (int count = 1; count <= range; count++) {
...
}
throw new AssertionError("unreachable code reached");
}
As @BoristheSpider pointed out you can make sure the second return
statement is semantically unreachable:
private static int oneRun(int range) {
int[] rInt = new int[range+1]; // Stores the past sequence of ints.
int count = 0;
while (true) {
rInt[count] = generator.nextInt(range); // Add randint to current iteration.
for (int i = 0; i < count; i++) { // Check for past occurence and return if found.
if (rInt[i] == rInt[count]) {
return count;
}
}
count++;
}
}
Compiles & runs fine. And if you ever get an ArrayIndexOutOfBoundsException
you'll know the implementation was semantically wrong, without having to explicitly throw anything.
Since you asked about breaking out of two for
loops, you can use a label to do that (see the example below):
private static int oneRun(int range) {
int returnValue=-1;
int[] rInt = new int[range+1]; // Stores the past sequence of ints.
rInt[0] = generator.nextInt(range); // Inital random number.
OUTER: for (int count = 1; count <= range; count++) { // Run until return.
rInt[count] = generator.nextInt(range); // Add randint to current iteration.
for (int i = 0; i < count; i++) { // Check for past occurence and return if found.
if (rInt[i] == rInt[count]) {
returnValue = count;
break OUTER;
}
}
}
return returnValue;
}
While an assert is a good fast solution. In general this kind of problems means that your code is too complicated. When I am looking at your code, it's obvious that you don't really want an array to hold previous numbers. You want a Set
:
Set<Integer> previous = new HashSet<Integer>();
int randomInt = generator.nextInt(range);
previous.add(randomInt);
for (int count = 1; count <= range; count++) {
randomInt = generator.nextInt(range);
if (previous.contains(randomInt)) {
break;
}
previous.add(randomInt);
}
return previous.size();
Now note that what we are returning is actually the size of the set. The code complexity has decreased from quadratic to linear and it is immediately more readable.
Now we can realize that we don't even need that count
index:
Set<Integer> previous = new HashSet<Integer>();
int randomInt = generator.nextInt(range);
while (!previous.contains(randomInt)) {
previous.add(randomInt);
randomInt = generator.nextInt(range);
}
return previous.size();