Wie vermeide ich eine nutzlose Rückgabe in einer Java-Methode?

115

Ich habe eine Situation, in der die returnin zwei forSchleifen verschachtelte Anweisung theoretisch immer erreicht wird.

Der Compiler ist anderer Meinung und benötigt eine returnAnweisung außerhalb der forSchleife. Ich würde gerne einen eleganten Weg kennen, um diese Methode zu optimieren, der über mein derzeitiges Verständnis hinausgeht, und keine meiner versuchten Implementierungen von break scheint zu funktionieren.

Im Anhang befindet sich eine Methode aus einer Zuweisung, die zufällige Ganzzahlen generiert und die durchlaufenen Iterationen zurückgibt, bis eine zweite zufällige Ganzzahl gefunden wird, die innerhalb eines Bereichs generiert wird, der als int-Parameter an die Methode übergeben wird.

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
quelle
9
Wenn das letzte Element nie erreicht wird, können Sie while(true)eine indizierte Schleife anstelle einer indizierten Schleife verwenden. Dies teilt dem Compiler mit, dass die Schleife niemals zurückkehren wird.
Boris die Spinne
101
Rufen Sie die Funktion mit dem Bereich 0 (oder einer anderen Zahl kleiner als 1) ( oneRun(0)) auf und Sie sehen, dass Sie schnell Ihre nicht erreichbare erreichenreturn
mcfedr
55
Die Rückgabe ist erreicht, wenn der gelieferte Bereich negativ ist. Sie haben auch eine 0-Validierung für den Eingabebereich, sodass Sie ihn derzeit nicht auf andere Weise abfangen.
Hoffentlich
4
@HopefullyHelpful Das ist natürlich die wahre Antwort. Es ist überhaupt keine nutzlose Rückkehr!
Herr Lister
4
@HopefullyHelpful nein, weil das nextInteine Ausnahme für wirft range < 0Der einzige Fall, in dem die Rückkehr erreicht wird, ist, wennrange == 0
njzk2

Antworten:

343

Mit den Heuristiken des Compilers können Sie niemals die letzte weglassen return. Wenn Sie sicher sind, dass es niemals erreicht wird, würde ich es durch ein ersetzen throw, um die Situation klarer zu machen.

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
quelle
135
Nicht nur die Lesbarkeit, sondern stellt auch sicher, dass Sie herausfinden, ob etwas mit Ihrem Code nicht stimmt. Es ist völlig plausibel, dass der Generator einen Fehler haben könnte.
JollyJoker
6
Wenn Sie ABSOLUT sicher sind, dass Ihr Code niemals zu diesem "nicht erreichbaren Code" gelangen kann, erstellen Sie einige Komponententests für alle möglichen Randfälle (Bereich ist 0, Bereich ist -1, Bereich ist min / max int). Es mag jetzt eine private Methode sein, aber der nächste Entwickler könnte es nicht so halten. Wie Sie mit unerwarteten Werten umgehen (eine Ausnahme auslösen, einen Fehlerwert zurückgeben, nichts tun), hängt davon ab, wie Sie die Methode verwenden. Nach meiner Erfahrung möchten Sie normalerweise nur einen Fehler protokollieren und zurückkehren.
Rick Ryker
22
Vielleicht sollte das seinthrow new AssertionError("\"unreachable\" code reached");
Bohemian
8
Ich würde genau das schreiben, was ich in meine Antwort in einem Produktionsprogramm eingegeben habe.
John Kugelman
1
Für das gegebene Beispiel gibt es einen besseren Weg, als einfach einen hinzuzufügen throw, der niemals erreicht werden kann. Viel zu oft wollen die Leute einfach schnell "eine Lösung anwenden, um sie alle zu regieren", ohne zu viel über das zugrunde liegende Problem nachzudenken. Programmieren ist jedoch viel mehr als nur Codieren. Besonders wenn es um Algorithmen geht. Wenn Sie das gegebene Problem (sorgfältig) untersuchen, werden Sie feststellen, dass Sie die letzte Iteration der äußeren forSchleife herausrechnen und somit die "nutzlose" Rückgabe durch eine nützliche ersetzen können (siehe diese Antwort ).
a_guest
36

Wie @BoristheSpider hervorhob, können Sie sicherstellen, dass die zweite returnAnweisung semantisch nicht erreichbar ist:

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

Kompiliert und läuft gut. Und wenn Sie jemals eine bekommen ArrayIndexOutOfBoundsException, wissen Sie, dass die Implementierung semantisch falsch war, ohne explizit etwas werfen zu müssen.

l0b0
quelle
1
Genau: Die Bedingung wird eigentlich nicht zur Steuerung der Schleife verwendet, daher sollte sie nicht vorhanden sein. Ich würde dies jedoch for(int count = 0; true; count++)stattdessen schreiben .
cMaster - wieder einzusetzen monica
3
@ Master: Sie können das weglassen true:for(int count = 0; ; count++) …
Holger
@ Holger Ah. Da war ich mir nicht sicher, weil es um Java geht, und ich habe diese Sprache seit Jahren nicht mehr angefasst, da ich mich viel mehr mit C / C ++ beschäftige. Als ich die Verwendung von sah while(true), dachte ich, dass Java in dieser Hinsicht vielleicht etwas strenger ist. Gut zu wissen, dass es keinen Grund zur Sorge gab ... Eigentlich mag ich die ausgelassene trueVersion viel besser: Es macht visuell klar, dass es absolut keine Bedingung gibt, um sie anzusehen :-)
cmaster - monica wieder herstellen
3
Ich würde es vorziehen, nicht zu "für" zu wechseln. Ein "while-true" ist ein offensichtliches Flag, von dem erwartet wird, dass der Block bedingungslos eine Schleife durchläuft, es sei denn und bis etwas im Inneren ihn bricht. Ein "für" mit einer ausgelassenen Bedingung kommuniziert nicht so klar; es könnte leichter übersehen werden.
Corrodias
1
@ l0b0 Die ganze Frage wäre wahrscheinlich besser für die Codeüberprüfung, wenn man bedenkt, dass die Warnung mit der Codequalität zusammenhängt.
Sulthan
18

Da Sie nach dem Ausbrechen von zwei forSchleifen gefragt haben , können Sie dazu ein Etikett verwenden (siehe Beispiel unten):

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
quelle
Würde dies nicht fehlschlagen, da returnValuees nicht initialisiert verwendet werden kann?
Punkte
1
Höchstwahrscheinlich. Das Ziel des Beitrags war es zu zeigen, wie man aus beiden Schleifen ausbricht, wie das Originalplakat danach gefragt hatte. Das OP war sich sicher, dass die Ausführung niemals das Ende der Methode erreichen würde. Daher kann das, was Sie aufgerufen haben, behoben werden, indem returnValue bei der Deklaration auf einen beliebigen Wert initialisiert wird.
David Choweller
4
Sind Labels nicht solche Dinge, die Sie besser vermeiden sollten?
Serverfrog
2
Ich glaube du denkst an gotos.
David Choweller
4
Beschriftungen in einer hohen Programmiersprache. Absolut barbarisch ...
xDaizu
13

Während eine Behauptung eine gute schnelle Lösung ist. Im Allgemeinen bedeutet diese Art von Problemen, dass Ihr Code zu kompliziert ist. Wenn ich mir Ihren Code anschaue, ist es offensichtlich, dass Sie nicht wirklich möchten, dass ein Array vorherige Nummern enthält. Du willst ein 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();

Beachten Sie nun, dass das, was wir zurückgeben, tatsächlich die Größe des Sets ist. Die Codekomplexität hat sich von quadratisch zu linear verringert und ist sofort besser lesbar.

Jetzt können wir erkennen, dass wir diesen countIndex nicht einmal brauchen :

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

Da Ihr Rückgabewert auf der Variablen der äußeren Schleife basiert, können Sie einfach den Zustand der äußeren Schleife ändern count < rangeund dann diesen letzten Wert (den Sie gerade weggelassen haben) am Ende der Funktion zurückgeben:

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

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

Auf diese Weise müssen Sie keinen Code einführen, der niemals erreicht wird.

ein Gast
quelle
Aber würde dies nicht ein Ausbrechen der beiden Schleifenebenen erfordern, wenn die Übereinstimmung gefunden wird? Ich sehe keine Möglichkeit, die Anweisung auf eins zu reduzieren. Eine neue Zufallszahl muss generiert und mit den vorherigen verglichen werden, bevor eine andere generiert wird.
Oliver Benning
Sie haben immer noch zwei verschachtelte for-Schleifen (ich habe gerade die zweite weggelassen ...), aber die äußere Schleife wurde durch ihre letzte Iteration reduziert. Der dieser letzten Iteration entsprechende Fall wird von der allerletzten return-Anweisung separat behandelt. Obwohl dies eine elegante Lösung für Ihr Beispiel ist, gibt es Szenarien, in denen dieser Ansatz schwieriger zu lesen ist. Wenn zum Beispiel der Rückgabewert von der inneren Schleife abhängt, bleibt anstelle einer einzelnen return-Anweisung am Ende eine zusätzliche Schleife übrig (die die innere Schleife für die letzte Iteration der äußeren Schleife darstellt).
a_guest
5

Verwenden Sie eine temporäre Variable, zum Beispiel "result", und entfernen Sie die innere Rückgabe. Ändern Sie die for-Schleife für eine while-Schleife unter den richtigen Bedingungen. Für mich ist es immer eleganter, nur eine Rückgabe als letzte Anweisung der Funktion zu haben.

David
quelle
Nun, sieht aus wie der sehr innere, wenn der äußere Zustand sein könnte. Denken Sie daran, dass es immer eine Weile gibt, die einem for entspricht (und eleganter ist, da Sie nicht vorhaben, immer alle Iterationen durchzugehen). Diese beiden verschachtelten for-Schleifen können sicher vereinfacht werden. Der ganze Code riecht "zu komplex".
David
@ Solomonoff'sSecret Deine Aussage ist absurd. Dies impliziert, dass wir "im Allgemeinen" zwei oder mehr Rückgabeanweisungen anstreben sollten. Interessant.
David
@ Solomonoff'sSecret Es ist eine Frage der bevorzugten Programmierstile, und unabhängig davon, was diese Woche cool war, was letzte Woche lahm war, gibt es definitiv Profis, nur einen Austrittspunkt zu haben und am Ende der Methode (denken Sie nur an eine Schleife mit Viele return resultstreuten sich darin herum und überlegten dann, ob sie das gefundene noch einmal überprüfen möchten, resultbevor sie es zurückgeben, und dies ist nur ein Beispiel. Es kann auch Nachteile geben, aber eine so breite Aussage wie "Im Allgemeinen gibt es keinen guten Grund, nur eine Rückkehr zu haben" hält kein Wasser.
SantiBailors
@ David Lassen Sie mich umformulieren: Es gibt keinen allgemeinen Grund, nur eine Rückkehr zu haben. In bestimmten Fällen könnte dies Gründe sein, aber normalerweise ist es der schlimmste Frachtkult.
Stellen Sie Monica
1
Es geht darum, was den Code lesbarer macht. Wenn Sie in Ihrem Kopf sagen: "Wenn dies der Fall ist, geben Sie das zurück, was wir gefunden haben. Fahren Sie ansonsten fort. Wenn wir nichts gefunden haben, geben Sie diesen anderen Wert zurück." Dann sollte Ihr Code zwei Rückgabewerte haben. Codieren Sie immer, was es für den nächsten Entwickler sofort verständlicher macht. Der Compiler hat nur einen Rückgabepunkt von einer Java-Methode, auch wenn er für uns wie zwei aussieht.
Rick Ryker
3

Vielleicht ist dies ein Hinweis darauf, dass Sie Ihren Code neu schreiben sollten. Beispielsweise:

  1. Erstellen Sie ein Array von Ganzzahlen 0 .. range-1. Setzen Sie alle Werte auf 0.
  2. Führen Sie eine Schleife durch. Generieren Sie in der Schleife eine Zufallszahl. Sehen Sie in Ihrer Liste nach diesem Index, um festzustellen, ob der Wert 1 ist. Wenn dies der Fall ist, brechen Sie aus der Schleife aus. Andernfalls setzen Sie den Wert an diesem Index auf 1
  3. Zählen Sie die Anzahl der Einsen in der Liste und geben Sie diesen Wert zurück.
Robert Hanson
quelle
3

Methoden mit einer return-Anweisung und einer Schleife / Schleifen erfordern immer eine return-Anweisung außerhalb der Schleife (n). Auch wenn diese Aussage außerhalb der Schleife nie erreicht wird. In solchen Fällen können Sie zu Beginn der Methode, dh vor und außerhalb der jeweiligen Schleife (n), eine Variable des jeweiligen Typs definieren, in Ihrem Fall eine Ganzzahl, um unnötige Rückgabeanweisungen zu vermeiden. Wenn das gewünschte Ergebnis innerhalb der Schleife erreicht ist, können Sie dieser vordefinierten Variablen den entsprechenden Wert zuweisen und ihn für die return-Anweisung außerhalb der Schleife verwenden.

Da Ihre Methode das erste Ergebnis zurückgeben soll, wenn rInt [i] gleich rInt [count] ist, reicht es nicht aus, nur die oben genannte Variable zu implementieren, da die Methode das letzte Ergebnis zurückgibt, wenn rInt [i] gleich rInt [count] ist. Eine Möglichkeit besteht darin, zwei "break-Anweisungen" zu implementieren, die aufgerufen werden, wenn das gewünschte Ergebnis erzielt wird. Die Methode sieht also ungefähr so ​​aus:

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

Ich bin damit einverstanden, dass eine Ausnahme ausgelöst wird, wenn eine nicht erreichbare Aussage auftritt. Ich wollte nur zeigen, wie dieselbe Methode dies besser lesbar macht (Java 8-Streams erforderlich).

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
quelle
-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
quelle
Der Code ist auch ineffizient , da man tun eine Menge von result == -1Schecks , die mit der weggelassen werden kann , um returndie Schleife im Inneren ...
Willem Van Onsem