Comment éviter un retour inutile dans une méthode Java?

115

J'ai une situation où le return déclaration imbriquée dans deux forboucles sera toujours atteinte, en théorie.

Le compilateur n'est pas d'accord et nécessite un return déclaration en dehors de la forboucle. J'aimerais connaître un moyen élégant d'optimiser cette méthode qui dépasse ma compréhension actuelle, et aucune de mes tentatives d'implémentation de break ne semble fonctionner.

Ci-joint une méthode à partir d'une affectation qui génère des entiers aléatoires et retourne les itérations parcourues jusqu'à ce qu'un deuxième entier aléatoire soit trouvé, généré dans une plage transmise à la méthode en tant que paramètre 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
la source
9
Si le dernier élément n'est jamais atteint, vous pouvez utiliser une while(true)boucle plutôt qu'une boucle indexée. Cela indique au compilateur que la boucle ne reviendra jamais.
Boris the Spider
101
appelez la fonction avec une plage de 0 (ou tout autre nombre inférieur à 1) ( oneRun(0)) et vous voyez que vous atteignez rapidement votre inaccessiblereturn
mcfedr
55
Le retour est atteint lorsque la plage fournie est négative. Vous avez également 0 validation pour la plage d'entrée, de sorte que vous ne l'attrapez actuellement d'aucune autre manière.
J'espère
4
@HopefullyHelpful C'est la vraie réponse, bien sûr. Ce n'est pas du tout un retour inutile!
Mr Lister
4
@HopefullyHelpful non, car le nextIntlève une exception pour range < 0Le seul cas où le retour est atteint est quandrange == 0
njzk2

Réponses:

343

L'heuristique du compilateur ne vous laissera jamais omettre le dernier return. Si vous êtes sûr qu'il ne sera jamais atteint, je le remplacerais par un throwpour clarifier la situation.

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
la source
135
Non seulement la lisibilité, mais vous permet de savoir s'il y a un problème avec votre code. Il est tout à fait plausible que le générateur ait un bug.
JollyJoker
6
Si vous êtes ABSOLUMENT sûr que votre code ne pourra jamais accéder à ce "code inaccessible", créez des tests unitaires pour tous les cas extrêmes qui pourraient survenir (la plage est 0, la plage est -1, la plage est min / max int). C'est peut-être une méthode privée maintenant, mais le prochain développeur pourrait ne pas le garder ainsi. La façon dont vous gérez les valeurs inattendues (lancer une exception, renvoyer une valeur d'erreur, ne rien faire) dépend de la façon dont vous allez utiliser la méthode. D'après mon expérience, vous souhaitez généralement simplement enregistrer une erreur et revenir.
Rick Ryker
22
Peut-être que cela devrait êtrethrow new AssertionError("\"unreachable\" code reached");
Bohème
8
J'écrirais exactement ce que j'ai mis dans ma réponse dans un programme de production.
John Kugelman
1
Pour l'exemple donné, il y a une meilleure façon de traiter que d'ajouter simplement un throwqui ne sera jamais atteint. Trop souvent, les gens veulent simplement appliquer rapidement «une solution pour les gouverner tous» sans trop penser au problème sous-jacent. Mais la programmation est bien plus qu'un simple codage. Surtout quand il s'agit d'algorithmes. Si vous inspectez (soigneusement) le problème donné, vous vous rendrez compte que vous pouvez factoriser la dernière itération de la forboucle externe et donc remplacer le retour "inutile" par un retour utile (voir cette réponse ).
a_guest
36

Comme @BoristheSpider l'a souligné, vous pouvez vous assurer que la deuxième returninstruction est sémantiquement inaccessible:

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

Compile et fonctionne bien. Et si jamais vous obtenez un, ArrayIndexOutOfBoundsExceptionvous saurez que l'implémentation était sémantiquement fausse, sans avoir à lancer quoi que ce soit explicitement.

l0b0
la source
1
Exactement: la condition n'est pas réellement utilisée pour contrôler la boucle, elle ne devrait donc pas être là. Cependant, j'écrirais ceci à la for(int count = 0; true; count++)place.
cmaster - réintégrer monica le
3
@cmaster: vous pouvez omettre true:for(int count = 0; ; count++) …
Holger
@Holger Ah. Je n'étais pas sûr de cela car il s'agit de java, et je n'ai pas touché à ce langage depuis des années car je suis beaucoup plus en C / C ++. Donc, quand j'ai vu l'utilisation de while(true), j'ai pensé que java était peut-être un peu plus strict à cet égard. C'est bien de savoir qu'il n'y avait pas lieu de s'inquiéter ... En fait, j'aime truebeaucoup mieux la version omise : cela montre clairement qu'il n'y a absolument aucune condition à regarder :-)
cmaster - réintégrer monica
3
Je préfère ne pas passer à "pour". Un "while-true" est un indicateur évident que le bloc est censé boucler inconditionnellement, à moins et jusqu'à ce que quelque chose à l'intérieur le casse. Un «pour» avec une condition omise ne communique pas aussi clairement; il pourrait être négligé plus facilement.
Corrodias
1
@ l0b0 Toute la question serait probablement meilleure pour la révision du code étant donné que l'avertissement est lié à la qualité du code.
Sulthan le
18

Puisque vous avez posé une question sur la rupture de deux forboucles, vous pouvez utiliser une étiquette pour le faire (voir l'exemple ci-dessous):

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
la source
Cela n'échouerait-il pas à compiler car il returnValuepeut être utilisé non initialisé?
pts
1
Probablement. Le but du message était de montrer comment sortir des deux boucles, comme l'affiche originale l'avait demandé. L'OP était sûr que l'exécution n'atteindrait jamais la fin de la méthode, donc ce que vous avez évoqué peut être corrigé en initialisant returnValue à n'importe quelle valeur lorsqu'elle est déclarée.
David Choweller
4
Les étiquettes ne sont-elles pas ce genre de choses qu'il vaut mieux éviter par aucun moyen?
Serverfrog
2
Je crois que vous pensez à gotos.
David Choweller
4
Étiquettes dans un langage de programmation de haut niveau (-ish). Absolument barbare ...
xDaizu
13

Alors qu'une affirmation est une bonne solution rapide. En général, ce genre de problèmes signifie que votre code est trop compliqué. Quand je regarde votre code, il est évident que vous ne voulez pas vraiment qu'un tableau contienne les numéros précédents. Vous voulez un 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();

Notez maintenant que ce que nous retournons est en fait la taille de l'ensemble. La complexité du code est passée de quadratique à linéaire et il est immédiatement plus lisible.

Maintenant, nous pouvons réaliser que nous n'avons même pas besoin de cet countindex:

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
la source
8

Comme votre valeur de retour est basée sur la variable de la boucle externe, vous pouvez simplement modifier la condition de la boucle externe, count < rangepuis renvoyer cette dernière valeur (que vous venez d'omettre) à la fin de la fonction:

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

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

De cette façon, vous n'avez pas besoin d'introduire du code qui ne sera jamais atteint.

un invité
la source
Mais cela ne nécessiterait-il pas une rupture des deux couches de boucles lorsque la correspondance est trouvée? Je ne vois pas de moyen de réduire à un pour l'instruction, un nouveau nombre aléatoire doit être généré et comparé à ceux avant qu'il ne soit généré.
Oliver Benning
Il vous reste encore deux boucles for imbriquées (j'ai simplement omis la seconde ...) mais la boucle externe a été réduite par sa dernière itération. Le cas correspondant à cette dernière itération est traité séparément par la toute dernière instruction return. Bien que ce soit une solution élégante pour votre exemple, il existe des scénarios dans lesquels cette approche devient plus difficile à lire; si, par exemple, la valeur de retour dépendait de la boucle interne, alors - au lieu d'une seule instruction de retour à la fin - vous vous retrouveriez avec une boucle supplémentaire (représentant la boucle interne de la dernière itération de la boucle externe).
a_guest
5

Utilisez une variable temporaire, par exemple "result", et supprimez le retour interne. Changez la boucle for pour une boucle while avec la bonne condition. Pour moi, il est toujours plus élégant de n'avoir qu'un seul retour comme dernière instruction de la fonction.

David
la source
Eh bien, on dirait que la condition très intérieure pourrait être la condition while extérieure. Gardez à l'esprit qu'il y a toujours un temps équivalent à un for (et plus élégant puisque vous ne prévoyez pas de toujours parcourir toutes les itérations). Ces deux boucles imbriquées pour peuvent être simplifiées à coup sûr. Tout ce code sent "trop ​​complexe".
David
@ Solomonoff'sSecret Votre déclaration est absurde. Cela implique que nous devrions "en général" viser deux ou plusieurs déclarations de retour. Intéressant.
David
@ Solomonoff'sSecret C'est une question de styles de programmation préférés, et peu importe ce qui est cool cette semaine qui était boiteux la semaine dernière, il y a des avantages certains à n'avoir qu'un seul point de sortie, et à la fin de la méthode (il suffit de penser à une boucle avec beaucoup ont return resultsaupoudré dedans et pensent ensuite vouloir faire une vérification de plus sur le trouvé resultavant de le retourner, et ce n'est qu'un exemple). Il peut aussi y avoir des inconvénients, mais une déclaration aussi large que "En général, il n'y a aucune bonne raison d'avoir un seul retour" ne tient pas la route.
SantiBailors
@David Permettez-moi de reformuler: il n'y a pas de raison générale d'avoir un seul retour. Dans des cas particuliers, les raisons peuvent être, mais c'est généralement le culte du fret à son pire.
Réintégrer Monica le
1
Il s'agit de savoir ce qui rend le code plus lisible. Si dans votre tête vous dites "si c'est ceci, retournez ce que nous avons trouvé; sinon continuez; si nous n'avons pas trouvé quelque chose, retournez cette autre valeur". Ensuite, votre code doit avoir deux valeurs de retour. Codez toujours ce qui le rend plus immédiatement compréhensible pour le prochain développeur. Le compilateur n'a qu'un seul point de retour d'une méthode Java même si à nos yeux il en ressemble à deux.
Rick Ryker
3

Peut-être que c'est une indication que vous devriez réécrire votre code. Par exemple:

  1. Créez un tableau d'entiers 0 .. range-1. Définissez toutes les valeurs sur 0.
  2. Effectuez une boucle. Dans la boucle, générez un nombre aléatoire. Regardez dans votre liste, à cet index, pour voir si la valeur est 1 Si c'est le cas, sortez de la boucle. Sinon, définissez la valeur de cet index sur 1
  3. Comptez le nombre de 1 dans la liste et renvoyez cette valeur.
Robert Hanson
la source
3

Les méthodes qui ont une instruction return et ont une ou des boucles à l'intérieur nécessitent toujours une instruction return en dehors de la ou des boucles. Même si cette instruction en dehors de la boucle n'est jamais atteinte. Dans de tels cas, afin d'éviter des déclarations de retour inutiles, vous pouvez définir une variable du type respectif, un entier dans votre cas, au début de la méthode, c'est-à-dire avant et en dehors de la ou des boucles respectives. Lorsque le résultat souhaité à l'intérieur de la boucle est atteint, vous pouvez attribuer la valeur respective à cette variable prédéfinie et l'utiliser pour l'instruction return en dehors de la boucle.

Puisque vous voulez que votre méthode renvoie le premier résultat lorsque rInt [i] est égal à rInt [count], implémenter uniquement la variable mentionnée ci-dessus n'est pas suffisant car la méthode retournera le dernier résultat lorsque rInt [i] est égal à rInt [count]. Une des options consiste à implémenter deux "instructions break" qui sont appelées lorsque nous obtenons le résultat souhaité. Ainsi, la méthode ressemblera à quelque chose comme ceci:

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
la source
2

Je suis d'accord qu'il faut lever une exception là où une déclaration inaccessible se produit. Je voulais juste montrer comment la même méthode peut faire cela de manière plus lisible (java 8 streams requis).

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
la source
-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
la source
Le code est également inefficace car on fera beaucoup de result == -1vérifications qui peuvent être omises avec l' returnintérieur de la boucle ...
Willem Van Onsem