Czy posiadanie instrukcji return tylko w celu spełnienia wymagań dotyczących składni jest złym rozwiązaniem?

Rozważ następujący kod:

public Object getClone(Cloneable a) throws TotallyFooException {

    if (a == null) {
        throw new TotallyFooException();
    }
    else {
        try {
            return a.clone();
        } catch (CloneNotSupportedException e) {
            e.printStackTrace();
        }
    }
    //cant be reached, in for syntax
    return null;
}

Jest return null;to konieczne, ponieważ wyjątek może zostać przechwycony, jednak w takim przypadku, ponieważ sprawdziliśmy już, czy jest pusty (i załóżmy, że wiemy, że klasa, którą wywołujemy, obsługuje klonowanie), więc wiemy, że instrukcja try nigdy nie zawiedzie.

Czy jest złą praktyką umieszczanie dodatkowej instrukcji return na końcu tylko po to, aby spełnić składnię i uniknąć błędów kompilacji (z komentarzem wyjaśniającym, że nie zostanie osiągnięty), czy też jest lepszy sposób na zakodowanie czegoś takiego, aby dodatkowe instrukcja zwrotu jest niepotrzebna?

Jaśniejszy sposób bez dodatkowej instrukcji powrotu jest następujący. Ja też bym nie złapał CloneNotSupportedException, ale zostawiłam dzwoniącemu.

if (a != null) {
    try {
        return a.clone();
    } catch (CloneNotSupportedException e) {
        e.printStackTrace();
    }
}
throw new TotallyFooException();

Prawie zawsze można bawić się kolejnością, aby uzyskać prostszą składnię niż ta, którą masz na początku.

Zdecydowanie można to osiągnąć. Zwróć uwagę, że drukujesz tylko ślad stosu w catchklauzuli.

W scenariuszu, w którym a != nulli będzie wyjątek, return null zostanie osiągnięty. Możesz usunąć to oświadczenie i zastąpić je throw new TotallyFooException();.

Ogólnie ^* , jeśli nulljest poprawnym wynikiem metody (tj. Użytkownik tego oczekuje i to coś znaczy), to zwrócenie go jako sygnału „nie znaleziono danych” lub wystąpił wyjątek nie jest dobrym pomysłem. W przeciwnym razie nie widzę problemu, dlaczego nie powinieneś wracać null.

Weźmy na przykład Scanner#ioExceptionmetodę:

Zwraca IOExceptionostatni rzucony przez Readable tego skanera. Ta metoda zwraca wartość null, jeśli nie istnieje taki wyjątek .

W tym przypadku zwracana wartość nullma wyraźne znaczenie, gdy używam metody mam pewność, że otrzymałem nulltylko dlatego, że nie było takiego wyjątku a nie dlatego, że metoda próbowała coś zrobić i się nie udało.

^* Zwróć uwagę, że czasami chcesz wrócić, nullnawet jeśli znaczenie jest niejednoznaczne. Na przykład HashMap#get:

Zwracana wartość null niekoniecznie oznacza, że ​​mapa nie zawiera żadnego mapowania dla klucza; jest również możliwe, że mapa jawnie mapuje klucz na null . containsKeyOperacja może być używany do rozróżnienia tych dwóch przypadkach.

W tym przypadku nullmoże wskazywać, że wartość null została znaleziona i zwrócona lub że hashmap nie zawiera żądanego klucza.

Czy jest złą praktyką umieszczanie dodatkowej instrukcji return na końcu tylko po to, aby spełnić składnię i uniknąć błędów kompilacji (z komentarzem wyjaśniającym, że nie zostanie osiągnięty)

Myślę, że return nullto zła praktyka na końcu niedostępnej gałęzi. Lepiej jest rzucić RuntimeException( AssertionErrorbyłoby to również dopuszczalne), ponieważ aby dostać się do tej linii, coś poszło bardzo źle i aplikacja jest w nieznanym stanie. Większość z nich jest (jak powyżej), ponieważ programista coś przeoczył (obiekty mogą być niezerowe i niemożliwe do sklonowania).

Prawdopodobnie nie użyłbym, InternalErrorchyba że jestem bardzo pewien, że kod jest nieosiągalny (na przykład po a System.exit()), ponieważ jest bardziej prawdopodobne, że popełnię błąd niż maszyna wirtualna.

Użyłbym niestandardowego wyjątku (takiego jak TotallyFooException) tylko wtedy, gdy przejście do tej „nieosiągalnej linii” oznacza to samo, co gdziekolwiek indziej, w którym rzucisz ten wyjątek.

Złapałeś CloneNotSupportedExceptionco oznacza, że ​​twój kod może sobie z tym poradzić. Ale kiedy go złapiesz, dosłownie nie masz pojęcia, co zrobić, gdy dojdziesz do końca funkcji, co oznacza, że ​​nie możesz sobie z tym poradzić. Więc masz rację, że w tym przypadku jest to zapach kodu i moim zdaniem oznacza, że ​​nie powinieneś był złapać CloneNotSupportedException.

Wolałbym użyć, Objects.requireNonNull()aby sprawdzić, czy parametr a nie jest pusty. Więc kiedy czytasz kod, jest jasne, że parametr nie powinien mieć wartości null.

Aby uniknąć zaznaczonych wyjątków, odrzuciłbym CloneNotSupportedExceptionjako RuntimeException.

W obu przypadkach możesz dodać ładny tekst z intencją, dlaczego tak się nie stanie lub tak się stanie.

public Object getClone(Object a) {

    Objects.requireNonNull(a);

    try {
        return a.clone();
    } catch (CloneNotSupportedException e) {
        throw new IllegalArgumentException(e);
    }

}

W takiej sytuacji napisałbym

public Object getClone(SomeInterface a) throws TotallyFooException {
    // Precondition: "a" should be null or should have a someMethod method that
    // does not throw a SomeException.
    if (a == null) {
        throw new TotallyFooException() ; }
    else {
        try {
            return a.someMethod(); }
        catch (SomeException e) {
            throw new IllegalArgumentException(e) ; } }
}

Co ciekawe, mówisz, że „instrukcja try nigdy nie zawiedzie”, ale nadal zadałeś sobie trud napisania oświadczenia, e.printStackTrace();które według Ciebie nigdy nie zostanie wykonane. Czemu?

Być może twoja wiara nie jest tak mocno zakorzeniona. To jest dobre (moim zdaniem), ponieważ twoja wiara nie jest oparta na kodzie, który napisałeś, ale raczej na oczekiwaniu, że twój klient nie naruszy warunku wstępnego. Lepiej programować metody publiczne defensywnie.

Nawiasem mówiąc, twój kod się nie skompiluje. Nie możesz zadzwonić, a.clone()nawet jeśli typ ajest Cloneable. Przynajmniej tak mówi kompilator Eclipse. Wyrażenie a.clone()powoduje błąd

Metoda clone () jest niezdefiniowana dla typu Cloneable

To, co zrobiłbym w twoim konkretnym przypadku, to

public Object getClone(PubliclyCloneable a) throws TotallyFooException {
    if (a == null) {
        throw new TotallyFooException(); }
    else {
        return a.clone(); }
}

Gdzie PubliclyCloneablejest zdefiniowane przez

interface PubliclyCloneable {
    public Object clone() ;
}

Lub, jeśli absolutnie potrzebujesz tego typu parametru Cloneable, przynajmniej poniższe elementy zostaną skompilowane.

public static Object getClone(Cloneable a) throws TotallyFooException {
//  Precondition: "a" should be null or point to an object that can be cloned without
// throwing any checked exception.
    if (a == null) {
        throw new TotallyFooException(); }
    else {
        try {
            return a.getClass().getMethod("clone").invoke(a) ; }
        catch( IllegalAccessException e ) {
            throw new AssertionError(null, e) ; }
        catch( InvocationTargetException e ) {
            Throwable t = e.getTargetException() ;
            if( t instanceof Error ) {
                // Unchecked exceptions are bubbled
                throw (Error) t ; }
            else if( t instanceof RuntimeException ) {
                // Unchecked exceptions are bubbled
                throw (RuntimeException) t ; }
            else {
                // Checked exceptions indicate a precondition violation.
                throw new IllegalArgumentException(t) ; } }
        catch( NoSuchMethodException e ) {
            throw new AssertionError(null, e) ; } }
}

Powyższe przykłady są poprawne i bardzo Java. Jednak oto, jak odniósłbym się do pytania OP, jak obsłużyć ten zwrot:

public Object getClone(Cloneable a) throws CloneNotSupportedException {
    return a.clone();
}

Nie ma korzyści ze sprawdzenia, aczy jest zerowa. Zmierza do NPE. Drukowanie śladu stosu również nie jest pomocne. Ślad stosu jest taki sam, niezależnie od tego, gdzie jest obsługiwany.

Nie ma korzyści z odrzucania kodu za pomocą niepomocnych testów zerowych i niepomocnej obsługi wyjątków. Usunięcie śmieci sprawia, że ​​kwestia zwrotu jest dyskusyjna.

(Zwróć uwagę, że OP zawiera błąd w obsłudze wyjątków; z tego powodu zwrot był potrzebny. OP nie pomyliłby metody, którą proponuję).

Czy posiadanie instrukcji return tylko w celu spełnienia wymagań dotyczących składni jest złym rozwiązaniem?

Jak wspominali inni, w twoim przypadku to nie ma zastosowania.

Aby odpowiedzieć na pytanie, programy typu Lint z pewnością tego nie rozgryzły! Widziałem dwóch różnych walczących o to w oświadczeniu przełącznika.

    switch (var)
   {
     case A:
       break;
     default:
       return;
       break;    // Unreachable code.  Coding standard violation?
   }

Jeden narzekał, że brak przerwy stanowił naruszenie standardu kodowania. Drugi narzekał, że posiadanie go było jednym, ponieważ był to nieosiągalny kod.

Zauważyłem to, ponieważ dwóch różnych programistów ponownie sprawdzało kod, dodając przerwę, a następnie usuwając ją, a następnie dodając i usuwając, w zależności od tego, który analizator kodu uruchomili tego dnia.

Jeśli znajdziesz się w takiej sytuacji, wybierz jedną i skomentuj anomalię, która jest dobrą formą, którą sobie pokazałeś. To najlepsze i najważniejsze danie na wynos.

Nie chodzi o „tylko zaspokojenie składni”. Warunkiem semantycznym języka jest to, że każda ścieżka kodu prowadzi do powrotu lub rzutu. Ten kod nie jest zgodny. Jeśli wyjątek zostanie przechwycony, wymagany jest następujący zwrot.

Żadnych „złych praktyk” w tym względzie, ani ogólnie dotyczących satysfakcji kompilatora.

W każdym razie, czy to składniowo, czy semantycznie, nie masz wyboru.

Przepisałbym to tak, aby na końcu miał powrót. Pseudo kod:

if a == null throw ...
// else not needed, if this is reached, a is not null
Object b
try {
  b = a.clone
}
catch ...

return b

Nikt jeszcze o tym nie wspomniał, więc oto:

public static final Object ERROR_OBJECT = ...

//...

public Object getClone(Cloneable a) throws TotallyFooException {
Object ret;

if (a == null) 
    throw new TotallyFooException();

//no need for else here
try {
    ret = a.clone();
} catch (CloneNotSupportedException e) {
    e.printStackTrace();
    //something went wrong! ERROR_OBJECT could also be null
    ret = ERROR_OBJECT; 
}

return ret;

}

Z tego właśnie powodu nie lubię returnwewnętrznych trybloków.

Zwrot null; jest konieczne, ponieważ wyjątek może zostać przechwycony, jednak w takim przypadku, ponieważ już sprawdziliśmy, czy jest on pusty (i załóżmy, że wiemy, że klasa, którą wywołujemy, obsługuje klonowanie), więc wiemy, że instrukcja try nigdy nie zawiedzie.

Jeśli znasz szczegóły dotyczące danych wejściowych w taki sposób, że wiesz, że trystwierdzenie nigdy nie zawiedzie, jaki jest sens jego posiadania? Unikaj tego, tryjeśli wiesz na pewno, że wszystko zawsze się powiedzie (choć rzadko można mieć całkowitą pewność przez cały okres istnienia kodu).

W każdym razie kompilator niestety nie jest czytnikiem w myślach. Widzi funkcję i jej dane wejściowe, a biorąc pod uwagę posiadane informacje, potrzebuje tego returnoświadczenia na dole, tak jak masz.

Czy jest złą praktyką umieszczanie dodatkowej instrukcji return na końcu tylko po to, aby spełnić składnię i uniknąć błędów kompilacji (z komentarzem wyjaśniającym, że nie zostanie osiągnięty), czy też jest lepszy sposób na zakodowanie czegoś takiego, aby dodatkowe instrukcja zwrotu jest niepotrzebna?

Wręcz przeciwnie, sugerowałbym dobrą praktykę unikanie jakichkolwiek ostrzeżeń kompilatora, np. Nawet jeśli kosztuje to kolejną linię kodu. Nie martw się tutaj zbytnio o liczbę linii. Ustal niezawodność funkcji poprzez testowanie, a następnie przejdź dalej. Po prostu udając, że możesz pominąć returnstwierdzenie, wyobraź sobie, że wracasz do tego kodu rok później, a następnie spróbuj zdecydować, czy to returnstwierdzenie na dole spowoduje więcej zamieszania niż komentarz szczegółowo wyjaśniający, dlaczego zostało pominięte z powodu przypuszczeń, które możesz zrobić o parametrach wejściowych. Najprawdopodobniej returnłatwiej będzie sobie z tym poradzić.

To powiedziawszy, szczególnie w tej części:

try {
    return a.clone();
} catch (CloneNotSupportedException e) {
   e.printStackTrace();
}
...
//cant be reached, in for syntax
return null;

Myślę, że jest coś nieco dziwnego w podejściu do obsługi wyjątków. Zwykle chcesz połknąć wyjątki w witrynie, w której masz coś znaczącego, co możesz zrobić w odpowiedzi.

Możesz myśleć o try/catchmechanizmie transakcji.trydokonanie tych zmian, jeśli zawiodą i rozgałęzimy się do catchbloku, zrób to (cokolwiek jest w catchbloku) w odpowiedzi jako część procesu wycofywania i odzyskiwania.

W tym przypadku samo wydrukowanie śladu stosu, a następnie zmuszenie do zwrócenia wartości null nie jest dokładnie nastawieniem do transakcji / odzyskiwania. Kod przenosi odpowiedzialność za obsługę błędów na wszystkie wywołania kodugetClone celu ręcznego sprawdzenia błędów. Możesz chcieć przechwycić CloneNotSupportedExceptioni przetłumaczyć go na inną, bardziej znaczącą formę wyjątku i wyrzucić go, ale nie chcesz po prostu połykać wyjątku i zwracać null w tym przypadku, ponieważ nie jest to witryna odzyskiwania transakcji.

Skończysz z wyciekiem obowiązków na dzwoniących, aby ręcznie sprawdzić i poradzić sobie z awarią w ten sposób, gdy rzucenie wyjątku pozwoliłoby tego uniknąć.

To tak, jakbyś załadował plik, to transakcja wysokiego poziomu. Możesz try/catchtam mieć . Podczas tryingładowania pliku można klonować obiekty. Jeśli wystąpi awaria w dowolnym miejscu tej operacji wysokiego poziomu (ładowania pliku), zazwyczaj chcesz zgłosić wyjątki z powrotem do tego try/catchbloku transakcji najwyższego poziomu , aby można było bezpiecznie odzyskać sprawność po niepowodzeniu ładowania pliku (niezależnie od tego, czy jest to z powodu błędu w klonowaniu lub czegokolwiek innego). Dlatego generalnie nie chcemy po prostu wchłaniać wyjątku w takim szczegółowym miejscu, jak to, a następnie zwracać wartość null, np. Ponieważ podważyłoby to wiele wartości i celu wyjątków. Zamiast tego chcemy propagować wyjątki z powrotem do witryny, w której możemy w znaczący sposób sobie z tym poradzić.

Twój przykład nie jest idealny do zilustrowania pytania, o którym mowa w ostatnim akapicie:

Czy jest złą praktyką umieszczanie dodatkowej instrukcji return na końcu tylko po to, aby spełnić składnię i uniknąć błędów kompilacji (z komentarzem wyjaśniającym, że nie zostanie osiągnięty), czy też jest lepszy sposób na zakodowanie czegoś takiego, aby dodatkowe instrukcja zwrotu jest niepotrzebna?

Lepszym przykładem może być implementacja samego klona:

 public class A implements Cloneable {
      public Object clone() {
           try {
               return super.clone() ;
           } catch (CloneNotSupportedException e) {
               throw new InternalError(e) ; // vm bug.
           }
      }
 }

Tutaj nigdy nie należy wpisywać klauzuli catch . Wciąż składnia wymaga albo rzucenia czegoś, albo zwrócenia wartości. Ponieważ zwracanie czegoś nie ma sensu, InternalErrorjest używany do wskazania poważnego stanu maszyny wirtualnej.