Como posso evitar um retorno inútil em um método Java?

115

Eu tenho uma situação onde o return instrução aninhada em dois forloops sempre será alcançada, teoricamente.

O compilador discorda e requer um return declaração fora do forloop. Eu gostaria de saber uma maneira elegante de otimizar esse método que está além do meu entendimento atual, e nenhuma das minhas tentativas de implementação de break parece funcionar.

Attached é um método de uma atribuição que gera inteiros aleatórios e retorna as iterações percorridas até que um segundo inteiro aleatório seja encontrado, gerado dentro de um intervalo passado para o método como um parâmetro 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
}
Oliver Benning
fonte
9
Se o último item nunca for alcançado, você pode usar um while(true)loop em vez de indexado. Isso informa ao compilador que o loop nunca retornará.
Boris the Spider
101
chame a função com intervalo igual a 0 (ou qualquer outro número menor que 1) ( oneRun(0)) e você verá que alcançará rapidamente o seu inacessívelreturn
mcfedr
55
O retorno é alcançado quando o intervalo fornecido é negativo. Você também tem validação 0 para o intervalo de entrada, portanto, não está captando-o de nenhuma outra maneira.
Esperançosamente útil
4
@HopefullyHelpful Essa é a verdadeira resposta, é claro. Não é um retorno inútil!
Sr. Lister
4
@HopefullyHelpful não, porque nextIntlança uma exceção para range < 0O único caso em que o retorno é alcançado é quandorange == 0
njzk2

Respostas:

343

A heurística do compilador nunca permitirá que você omita o último return. Se você tiver certeza de que nunca será alcançado, eu o substituo por um throwpara esclarecer a situação.

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");
}
John Kugelman
fonte
135
Não apenas legibilidade, mas garante que você descubra se há algo errado com seu código. É completamente plausível que o gerador possa ter um bug.
JollyJoker
6
Se você estiver ABSOLUTAMENTE certo de que seu código nunca poderá chegar a esse "código inacessível", crie alguns testes de unidade para todos os casos extremos que possam surgir (o intervalo é 0, o intervalo é -1, o intervalo é min / max int). Pode ser um método privado agora, mas o próximo desenvolvedor pode não mantê-lo assim. Como você lida com valores inesperados (lançar uma exceção, retornar um valor de erro, não fazer nada) depende de como você usará o método. Na minha experiência, geralmente você só deseja registrar um erro e retornar.
Rick Ryker
22
Talvez devesse serthrow new AssertionError("\"unreachable\" code reached");
Boêmio
8
Eu escreveria exatamente o que colocaria em minha resposta em um programa de produção.
John Kugelman
1
Para o exemplo dado, há uma maneira melhor de lidar com o que simplesmente adicionar um throwque nunca será alcançado. Com muita frequência, as pessoas apenas desejam aplicar rapidamente "uma solução para governar todos", sem pensar muito sobre o problema subjacente. Mas programar é muito mais do que apenas codificação. Especialmente quando se trata de algoritmos. Se você inspecionar (cuidadosamente) o problema fornecido, perceberá que pode fatorar a última iteração do forloop externo e, portanto, substituir o retorno "inútil" por um útil (veja esta resposta ).
a_guest
36

Como @BoristheSpider apontou, você pode garantir que a segunda returninstrução seja semanticamente inacessível:

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

Compila e funciona bem. E se você receber um, ArrayIndexOutOfBoundsExceptionsaberá que a implementação estava semanticamente errada, sem ter que lançar nada explicitamente.

l0b0
fonte
1
Exatamente: a condição não é realmente usada para controlar o loop, então não deveria estar lá. No entanto, eu escreveria isso como for(int count = 0; true; count++).
cmaster - restabelecer monica em
3
@cmaster: você pode omitir true:for(int count = 0; ; count++) …
Holger
@Holger Ah. Eu não tinha certeza disso porque se trata de java, e não toco nessa linguagem há anos, já que estou muito mais interessado em C / C ++. Então, quando vi o uso de while(true), pensei que talvez java seja um pouco mais rígido nesse aspecto. É bom saber que não havia necessidade de se preocupar ... Na verdade, eu gosto truemuito mais da versão omitida : torna visualmente claro que não há absolutamente nenhuma condição para olhar para :-)
cmaster - reintegrar monica
3
Prefiro não mudar para "para". Um "while-true" é uma sinalização óbvia de que se espera que o bloco faça um loop incondicional, a menos e até que algo dentro dele quebre. Um "para" com uma condição omitida não se comunica tão claramente; poderia ser esquecido mais facilmente.
Corrodias
1
@ l0b0 Toda a questão provavelmente seria melhor para a revisão do código, considerando que o aviso está relacionado à qualidade do código.
Sulthan
18

Como você perguntou sobre a quebra de dois forloops, você pode usar um rótulo para fazer isso (veja o exemplo abaixo):

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;
}
David Choweller
fonte
Não returnValuehaveria falha na compilação porque pode ser usado não inicializado?
pontos de
1
Provavelmente. O objetivo da postagem era mostrar como quebrar os dois loops, já que o autor original havia perguntado sobre isso. O OP tinha certeza de que a execução nunca chegaria ao final do método, então o que você trouxe pode ser corrigido inicializando returnValue com qualquer valor quando for declarado.
David Choweller
4
não são rótulos esse tipo de coisa que é melhor você evitar de alguma forma?
Serverfrog
2
Eu acredito que você está pensando em gotos.
David Choweller
4
Rótulos em uma linguagem de programação de alto (-ish) nível. Absolutamente bárbaro ...
xDaizu
13

Enquanto uma afirmação é uma boa solução rápida. Em geral, esse tipo de problema significa que seu código é muito complicado. Quando estou olhando para o seu código, é óbvio que você realmente não deseja que uma matriz contenha os números anteriores. Você quer um 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();

Agora observe que o que estamos retornando é, na verdade, o tamanho do conjunto. A complexidade do código diminuiu de quadrática para linear e é imediatamente mais legível.

Agora podemos perceber que nem mesmo precisamos desse countíndice:

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
fonte
8

Como seu valor de retorno é baseado na variável do loop externo, você pode simplesmente alterar a condição do loop externo count < rangee retornar este último valor (que você acabou de omitir) no final da função:

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

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

Dessa forma, você não precisa introduzir um código que nunca será alcançado.

um convidado
fonte
Mas isso não exigiria uma quebra das duas camadas de loops quando a correspondência fosse encontrada? Não vejo uma maneira de reduzir para uma instrução for, um novo número aleatório precisa ser gerado e comparado com os anteriores antes que outro seja gerado.
Oliver Benning
Você ainda tem dois loops for aninhados (apenas omiti o segundo ...), mas o loop externo foi reduzido por sua última iteração. O caso correspondente a esta última iteração é tratado separadamente pela última instrução return. Embora esta seja uma solução elegante para o seu exemplo, existem cenários em que essa abordagem se torna mais difícil de ler; se, por exemplo, o valor de retorno dependesse do loop interno, então - em vez de uma única instrução de retorno no final - você ficaria com um loop adicional (representando o loop interno da última iteração do loop externo).
a_guest
5

Use uma variável temporária, por exemplo "resultado", e remova o retorno interno. Altere o loop for por um loop while com a condição adequada. Para mim é sempre mais elegante ter apenas um retorno como a última instrução da função.

David
fonte
Bem, parece que o interior se poderia ser a condição while externa. Lembre-se de que sempre há um tempo equivalente a um for (e mais elegante, já que você não está planejando passar sempre por todas as iterações). Esses dois loops for aninhados podem ser simplificados com certeza. Todo aquele código cheira "muito complexo".
David
@Solomonoff'sSecret Sua declaração é absurda. Isso implica que devemos "em geral" ter como objetivo duas ou mais declarações de retorno. Interessante.
David
@Solomonoff'sSecret É uma questão de estilos de programação preferidos e, independentemente do que seja legal esta semana que foi ruim na semana passada, existem vantagens definitivas em ter apenas um ponto de saída e no final do método (basta pensar em um loop com muitos return resultespalhados dentro dele e então pensam em querer fazer mais uma verificação no achado resultantes de devolvê-lo, e este é apenas um exemplo). Também pode haver contras, mas uma declaração tão ampla como "Em geral, não há uma boa razão para ter apenas um retorno" não é válida.
SantiBailors de
@David Deixe-me reformular: não há nenhuma razão geral para ter apenas um retorno. Em casos particulares, podem haver razões, mas normalmente é o culto à carga no seu pior.
Reintegrar Monica em
1
É uma questão de o que torna o código mais legível. Se na sua cabeça você disser "se é isso, devolva o que encontramos; caso contrário, continue; se não encontramos algo devolva este outro valor". Então, seu código deve ter dois valores de retorno. Sempre codifique o que o torna mais imediatamente compreensível para o próximo desenvolvedor. O compilador tem apenas um ponto de retorno de um método Java, mesmo que aos nossos olhos pareça dois.
Rick Ryker
3

Talvez isso seja uma indicação de que você deve reescrever seu código. Por exemplo:

  1. Crie uma matriz de inteiros 0 .. intervalo-1. Defina todos os valores como 0.
  2. Execute um loop. No loop, gere um número aleatório. Olhe em sua lista, naquele índice, para ver se o valor é 1. Se for, saia do loop. Caso contrário, defina o valor nesse índice para 1
  3. Conte o número de 1s na lista e retorne esse valor.
Robert Hanson
fonte
3

Os métodos que possuem uma instrução de retorno e um loop / loops dentro deles sempre exigem uma instrução de retorno fora do (s) loop (s). Mesmo que essa instrução fora do loop nunca seja alcançada. Nesses casos, para evitar declarações de retorno desnecessárias, você pode definir uma variável do respectivo tipo, um inteiro no seu caso, no início do método, ou seja, antes e fora do (s) respectivo (s) loop (s). Quando o resultado desejado dentro do loop for alcançado, você pode atribuir o respectivo valor a essa variável predefinida e usá-lo para a instrução de retorno fora do loop.

Como você deseja que seu método retorne o primeiro resultado quando rInt [i] for igual a rInt [contagem], implementar apenas a variável mencionada acima não é suficiente porque o método retornará o último resultado quando rInt [i] for igual a rInt [contagem]. Uma das opções é implementar duas "instruções de interrupção" que são chamadas quando temos o resultado desejado. Portanto, o método será mais ou menos assim:

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
fonte
2

Concordo que se deve lançar uma exceção onde ocorre uma declaração inacessível. Só queria mostrar como o mesmo método pode fazer isso de maneira mais legível (streams java 8 necessários).

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!"));
}
Sergey Fedorov
fonte
-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
}
blabla
fonte
O código também é ineficiente, pois um vai fazer um monte de result == -1cheques que podem ser omitidos com o returndentro do loop ...
Willem Van Onsem