Как избежать бесполезного возврата в методе Java?

115

У меня есть ситуация, когда returnоператор, вложенный в два цикла for, всегда будет достигнут теоретически.

Компилятор не согласен и требует returnутверждения вне forцикла. Я хотел бы знать элегантный способ оптимизации этого метода, который выходит за рамки моего текущего понимания, и ни одна из моих попыток реализации break, похоже, не работает.

Прикрепленный - это метод из присваивания, который генерирует случайные целые числа и возвращает итерации, циклически выполняемые до тех пор, пока не будет найдено второе случайное целое число, сгенерированное в пределах диапазона, переданного в метод как параметр int.

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
}
Оливер Беннинг
источник
9
Если последний элемент никогда не достигается, вы можете использовать while(true)цикл, а не индексированный. Это сообщает компилятору, что цикл никогда не вернется.
Паук Борис
101
вызовите функцию с диапазоном 0 (или любым другим числом меньше 1) ( oneRun(0)), и вы увидите, что быстро достигнете своего недостижимого местаreturn
mcfedr
55
Возврат достигается, когда поставляемый диапазон отрицательный. У вас также есть проверка 0 для входного диапазона, поэтому в настоящее время вы не поймаете его каким-либо другим способом.
HopefullyHelpful
4
@HopefullyHelpful Это, конечно, настоящий ответ. Это вовсе не бесполезный возврат!
Мистер Листер,
4
@HopefullyHelpful нет, потому что nextIntгенерирует исключение для range < 0Единственный случай, когда достигается возврат, когдаrange == 0
njzk2

Ответы:

343

Эвристика компилятора никогда не позволит пропустить последнее return. Если вы уверены, что этого никогда не произойдет, я бы заменил его throwна, чтобы прояснить ситуацию.

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");
}
Джон Кугельман
источник
135
Не только читабельность, но и уверенность в том, что вы обнаружите, что с вашим кодом что-то не так. Вполне вероятно, что в генераторе может быть ошибка.
JollyJoker
6
Если вы АБСОЛЮТНО уверены, что ваш код никогда не сможет добраться до этого «недостижимого кода», тогда создайте несколько модульных тестов для всех крайних случаев, которые могут возникнуть (диапазон равен 0, диапазон равен -1, диапазон равен min / max int). Сейчас это может быть частный метод, но следующий разработчик может не оставить его таким. Как вы обрабатываете неожиданные значения (генерируете исключение, возвращаете значение ошибки, ничего не делаете) зависит от того, как вы собираетесь использовать метод. По моему опыту, вы обычно просто хотите зарегистрировать ошибку и вернуться.
Рик Райкер,
22
Может, так и должно бытьthrow new AssertionError("\"unreachable\" code reached");
Bohemian
8
Я бы написал именно то, что вложил в свой ответ, в производственной программе.
Джон Кугельман
1
Для данного примера есть лучший способ справиться с этим, чем просто добавить throw, который никогда не будет достигнут. Слишком часто люди просто хотят быстро применить «одно решение, чтобы управлять всеми ими», не слишком задумываясь о глубинной проблеме. Но программирование - это гораздо больше, чем просто кодирование. Особенно если речь идет об алгоритмах. Если вы (внимательно) изучите данную проблему, вы поймете, что можете исключить последнюю итерацию внешнего forцикла и, следовательно, заменить «бесполезный» возврат на полезный (см. Этот ответ ).
a_guest 07
36

Как указал @BoristheSpider, вы можете убедиться, что второй returnоператор семантически недоступен:

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

Компилируется и работает нормально. И если вы когда-нибудь получите сообщение, ArrayIndexOutOfBoundsExceptionвы узнаете, что реализация была семантически неверной, без необходимости явно бросать что-либо.

l0b0
источник
1
Точно: условие фактически не используется для управления циклом, поэтому его там не должно быть. Однако for(int count = 0; true; count++)вместо этого я бы написал это как .
cmaster - восстановить Монику
3
@cmaster: вы можете опустить true:for(int count = 0; ; count++) …
Holger
@ Холгер А. Я не был уверен в этом, потому что речь идет о java, и я не касался этого языка в течение многих лет, так как я больше увлекаюсь C / C ++. Итак, когда я увидел использование while(true), я подумал, что, возможно, java немного строже в этом отношении. Приятно знать, что волноваться не о чем ... На самом деле, мне trueгораздо больше нравится пропущенная версия: она дает визуально ясно, что нет абсолютно никаких условий, на которые можно смотреть :-)
cmaster - восстановить
3
Я бы предпочел не переходить на «за». «While-true» - это очевидный признак того, что блок должен безоговорочно зацикливаться, пока что-то внутри его не сломает. «For» с пропущенным условием не так ясно передается; его легче было не заметить.
Корродиас
1
@ l0b0 Весь вопрос, вероятно, лучше для проверки кода, учитывая, что предупреждение связано с качеством кода.
Sulthan
18

Поскольку вы спросили о выходе из двух for циклов, вы можете использовать для этого метку (см. Пример ниже):

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;
}
Дэвид Чоуллер
источник
Разве это не скомпилируется, потому что returnValueего можно использовать без инициализации?
Очков
1
Наверняка. Целью поста было показать, как разорвать обе петли, как об этом спрашивал оригинальный плакат. ОП был уверен, что выполнение никогда не дойдет до конца метода, поэтому то, что вы подняли, можно исправить, инициализировав returnValue любым значением при его объявлении.
Дэвид Чоуллер
4
Разве ярлыки не из тех вещей, которых лучше избегать?
Serverfrog
2
Я полагаю, вы думаете о gotos.
Дэвид Чоуллер,
4
Ярлыки на языке программирования высокого (-ского) уровня. Абсолютно варварски ...
xDaizu
13

Хотя assert - хорошее быстрое решение. В общем, такого рода проблемы означают, что ваш код слишком сложен. Когда я смотрю на ваш код, становится очевидно, что вы действительно не хотите, чтобы массив содержал предыдущие числа. Вы хотите 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();

Теперь обратите внимание, что на самом деле мы возвращаем размер набора. Сложность кода снизилась с квадратичной до линейной, и он сразу стал более читаемым.

Теперь мы можем понять, что нам даже не нужен этот countиндекс:

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();
Sulthan
источник
8

Поскольку ваше возвращаемое значение основано на переменной внешнего цикла, вы можете просто изменить условие внешнего цикла, count < rangeа затем вернуть это последнее значение (которое вы только что пропустили) в конце функции:

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

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

Таким образом, вам не нужно вводить код, который никогда не будет достигнут.

Гость
источник
Но разве для этого не потребуется разрыв двух слоев петель при обнаружении совпадения? Я не вижу способа сократить до одного оператора for, необходимо сгенерировать новое случайное число и сравнить его с предыдущими, прежде чем будет сгенерировано другое.
Оливер Беннинг,
У вас все еще осталось два вложенных ...цикла for (я просто пропустил второй ), но внешний цикл был сокращен к его последней итерации. Случай, соответствующий этой последней итерации, обрабатывается отдельно самым последним оператором return. Хотя это элегантное решение для вашего примера, существуют сценарии, в которых этот подход становится труднее читать; если, например, возвращаемое значение зависело от внутреннего цикла, тогда - вместо единственного оператора возврата в конце - у вас останется дополнительный цикл (представляющий внутренний цикл для последней итерации внешнего цикла).
a_guest
5

Используйте временную переменную, например «результат», и удалите внутренний возврат. Измените цикл for на цикл while с правильным условием. Для меня всегда более элегантно иметь только один возврат в качестве последнего оператора функции.

Дэвид
источник
Что ж, похоже, что внутреннее if могло быть внешним условием while. Имейте в виду, что всегда есть время, эквивалентное for (и более элегантно, поскольку вы не планируете всегда проходить все итерации). Эти два вложенных цикла for наверняка можно упростить. Весь этот код пахнет «слишком сложным».
Дэвид
@ Solomonoff'sSecret Ваше утверждение абсурдно. Это означает, что мы должны «в целом» стремиться к двум или более операторам возврата. Интересный.
Дэвид
@ Solomonoff'sSecret Это вопрос предпочтительных стилей программирования, и независимо от того, что круто на этой неделе, что было неуместно на прошлой неделе, есть определенные плюсы в том, чтобы иметь только одну точку выхода, и в конце метода (просто подумайте о цикле с многие из них return resultразбросаны по нему, а затем думают, что теперь хотят сделать еще одну проверку найденного, resultпрежде чем возвращать его, и это всего лишь пример). Могут быть и минусы, но такое широкое утверждение, как «В общем, нет веских причин иметь только один возврат» не выдерживает критики.
SantiBailors
@David Позвольте мне перефразировать: нет общей причины возвращать только один раз. В определенных случаях могут быть причины, но, как правило, это культ карго в худшем случае.
Восстановить Монику
1
Вопрос в том, что делает код более читабельным. Если в голове вы говорите «если это так, верните то, что мы нашли; в противном случае продолжайте; если мы не нашли что-то, верните это другое значение». Тогда ваш код должен иметь два возвращаемых значения. Всегда кодируйте то, что делает его более понятным для следующего разработчика. У компилятора есть только одна точка возврата из Java-метода, даже если на наш взгляд она выглядит как две.
Рик Райкер,
3

Может быть, это признак того, что вам следует переписать свой код. Например:

  1. Создайте массив целых чисел 0 .. диапазон-1. Установите все значения на 0.
  2. Выполните петлю. В цикле сгенерируйте случайное число. Посмотрите в своем списке на этот индекс, чтобы увидеть, равно ли значение 1. Если это так, выйдите из цикла. В противном случае установите значение этого индекса равным 1
  3. Подсчитайте количество единиц в списке и верните это значение.
Роберт Хэнсон
источник
3

Методы, которые имеют оператор возврата и внутри них цикл / циклы, всегда требуют оператора возврата вне цикла (ов). Даже если этот оператор вне цикла никогда не достигается. В таких случаях, чтобы избежать ненужных операторов возврата, вы можете определить переменную соответствующего типа, целое число в вашем случае, в начале метода, то есть до и за пределами соответствующего цикла (ов). Когда желаемый результат внутри цикла будет достигнут, вы можете присвоить соответствующее значение этой предопределенной переменной и использовать его для оператора возврата вне цикла.

Поскольку вы хотите, чтобы ваш метод возвращал первый результат, когда rInt [i] равняется rInt [count], реализации только вышеупомянутой переменной недостаточно, потому что метод вернет последний результат, когда rInt [i] равно rInt [count]. Один из вариантов - реализовать два «оператора прерывания», которые вызываются, когда мы получаем желаемый результат. Итак, метод будет выглядеть примерно так:

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;
    }
Lachezar
источник
2

Я согласен с тем, что следует генерировать исключение при возникновении недостижимого оператора. Просто хотел показать, как тот же метод может сделать это более читаемым (требуются потоки java 8).

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!"));
}
Сергей Федоров
источник
-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
}
блабла
источник
Код тоже неэффективен, так как много будет делать из result == -1проверок , которые могут быть опущены с returnвнутренней стороны петли ...
Willem Van Onsem