CODEWITHSUNDEEP
java
Oliver Benning
Oliver Benning

Reputation: 1281

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
}

Upvotes: 116

Views: 14400

Answers (10)

a_guest
a_guest

Reputation: 36249

As your return value is based on the outer loop's variable you could simply alter the outer loop's condition to count < range and then return this last value (which you've just omitted) at the end of the function:

private static int oneRun(int range) {
    ...

    for (int count = 1; count < range; count++) {
        ...
    }
    return range;
}

This way you don't need to introduce code that will never be reached.

Upvotes: 8

Sergey Fedorov
Sergey Fedorov

Reputation: 2169

I agree that one should throw an exception where unreachable statement occurs. Just wanted to show how the same method can do this in more readable way (java 8 streams required).

private static int oneRun(int range) {
    int[] rInt = new int[range + 1];
    return IntStream
        .rangeClosed(0, range)
        .peek(i -> rInt[i] = generator.nextInt(range))
        .filter(i -> IntStream.range(0, i).anyMatch(j -> rInt[i] == rInt[j]))
        .findFirst()
        .orElseThrow(() -> new RuntimeException("Shouldn't be reached!"));
}

Upvotes: 2

blabla
blabla

Reputation: 1

private static int oneRun(int range) {
    int result = -1; // use this to store your result
    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 && result == -1; count++) { // Run until result found.
        rInt[count] = generator.nextInt(range); // Add randint to current iteration.   
        for (int i = 0; i < count && result == -1; i++) { // Check for past occurence and leave after result found.
            if (rInt[i] == rInt[count]) {
                result = count;
            }
        }
    }
    return result; // return your result
}

Upvotes: -1

Lachezar
Lachezar

Reputation: 1

Methods that have a return statement and have a loop/loops inside them always require a return statement outside the loop(s). Even if this statement outside the loop is never reached. In such cases, in order to avoid unnecessary return statements, you could define a variable of the respective type, an integer in your case, at the beginning of the method i.e. before and outside the respective loop(s). When the desired result inside the loop is reached, you can ascribe the respective value to this pre-defined variable and use it for the return statement outside the loop.

Since you want your method to return the first result when rInt[i] equals rInt[count], implementing only the above-mentioned variable is not enough because the method will return the last result when rInt[i] equals rInt[count]. One options is to implement two "break statements" that are called when the we have the desired result. So, the method will look something like this:

private static int oneRun(int range) {

        int finalResult = 0; // the above-mentioned variable
        int[] rInt = new int[range + 1];
        rInt[0] = generator.nextInt(range);

        for (int count = 1; count <= range; count++) {
            rInt[count] = generator.nextInt(range);
            for (int i = 0; i < count; i++) {
                if (rInt[i] == rInt[count]) {
                    finalResult = count;
                    break; // this breaks the inside loop
                }
            }
            if (finalResult == count) {
                break; // this breaks the outside loop
            }
        }
        return finalResult;
    }

Upvotes: 3

Sulthan
Sulthan

Reputation: 130102

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();

Upvotes: 13

Robert Hanson
Robert Hanson

Reputation: 81

Maybe this is an indication that you should rewrite your code. For example:

  1. Create an array of integers 0 .. range-1. Set all the values to 0.
  2. Perform a loop. In the loop, generate a random number. Look in your list, at that index, to see if the value is 1 If it is, break out of the loop. Otherwise, set the value at that index to 1
  3. Count the number of 1s in the list, and return that value.

Upvotes: 3

David Choweller
David Choweller

Reputation: 1050

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;
}

Upvotes: 18

l0b0
l0b0

Reputation: 58798

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.

Upvotes: 36

David
David

Reputation: 69

Use a temp variable, for instance "result" , and remove the inner return. Change the for loop for a while loop with the proper condition. To me it's always more elegant to have only one return as the last statement of the function.

Upvotes: 5

John Kugelman
John Kugelman

Reputation: 361605

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");
}

Upvotes: 344

Related Questions