Czy zgłaszanie wyjątku jest tu anty-wzorem?

Właśnie omówiłem wybór projektu po przejrzeniu kodu. Zastanawiam się, jakie są twoje opinie.

Jest ta Preferencesklasa, która jest wiadrem dla par klucz-wartość. Wartości zerowe są legalne (to ważne). Oczekujemy, że niektóre wartości mogą jeszcze nie zostać zapisane i chcemy automatycznie obsługiwać te przypadki, inicjując je z predefiniowaną wartością domyślną na żądanie.

W omawianym rozwiązaniu zastosowano następujący wzorzec (UWAGA: oczywiście nie jest to rzeczywisty kod - oczywiście jest uproszczony w celach ilustracyjnych):

public class Preferences {
    // null values are legal
    private Map<String, String> valuesFromDatabase;

    private static Map<String, String> defaultValues;

    class KeyNotFoundException extends Exception {
    }

    public String getByKey(String key) {
        try {
            return getValueByKey(key);
        } catch (KeyNotFoundException e) {
            String defaultValue = defaultValues.get(key);
            valuesFromDatabase.put(key, defaultvalue);
            return defaultValue;
        }
    }

    private String getValueByKey(String key) throws KeyNotFoundException {
        if (valuesFromDatabase.containsKey(key)) {
            return valuesFromDatabase.get(key);
        } else {
            throw new KeyNotFoundException();
        }
    }
}

Został skrytykowany jako anty-wzór - nadużywanie wyjątków w celu kontrolowania przepływu . KeyNotFoundException- wprowadzone w życie tylko dla tego jednego przypadku użycia - nigdy nie będzie postrzegane poza zakresem tej klasy.

Zasadniczo są to dwie metody zabawy z pobieraniem go, po prostu komunikowanie sobie czegoś.

Klucz nieobecny w bazie danych nie jest czymś niepokojącym ani wyjątkowym - spodziewamy się, że tak się stanie, ilekroć zostanie dodane nowe ustawienie preferencji, stąd mechanizm, który w razie potrzeby z wdziękiem inicjuje wartość domyślną.

Kontrargumentem było to, że getValueByKey- metoda prywatna - jak zdefiniowano w tej chwili, nie ma naturalnego sposobu informowania metody publicznej zarówno o wartości, jak io tym, czy klucz tam był. (Jeśli nie, należy go dodać, aby można było zaktualizować wartość).

Powrót nullbyłby niejednoznaczny, ponieważ nulljest to całkowicie legalna wartość, więc nie wiadomo, czy oznaczało to, że klucza nie było, czy też był null.

getValueByKeymusiałby zwrócić jakiś a Tuple<Boolean, String>, bool ustawiony na true, jeśli klucz już tam jest, abyśmy mogli odróżnić od (true, null)i (false, null). (W języku outC # można użyć parametru, ale to jest Java).

Czy to ładniejsza alternatywa? Tak, musiałbyś zdefiniować jakąś klasę jednorazowego użytku Tuple<Boolean, String>, ale wtedy się pozbywamy KeyNotFoundException, więc ten rodzaj równowagi się wyrówna. Unikamy również narzutu związanego z obsługą wyjątku, chociaż nie jest to istotne z praktycznego punktu widzenia - nie ma tu żadnych wzmianek o wydajności, jest to aplikacja kliencka i to nie tak, że preferencje użytkownika będą pobierane miliony razy na sekundę.

Odmiana tego podejścia może używać Guawy Optional<String>(Guawa jest już używana w całym projekcie) zamiast jakiegoś niestandardowego Tuple<Boolean, String>, a następnie możemy rozróżnić Optional.<String>absent()i „właściwe” null. Nadal jednak wydaje się hacking z powodów, które są łatwe do zauważenia - wprowadzenie dwóch poziomów „nieważności” wydaje się nadużywać koncepcji stojącej za tworzeniem Optionals na pierwszym miejscu.

Inną opcją byłoby jawne sprawdzenie, czy klucz istnieje (dodaj boolean containsKey(String key)metodę i wywołaj getValueByKeytylko, jeśli już stwierdziliśmy, że istnieje).

Na koniec można również wstawić metodę prywatną, ale rzeczywista getByKeyjest nieco bardziej złożona niż mój przykładowy kod, więc wstawianie mogłoby sprawić, że wyglądałaby dość paskudnie.

Być może dzielę włosy tutaj, ale jestem ciekawy, na co postawiłbyś się, aby być najbliższym najlepszej praktyki w tym przypadku. Nie znalazłem odpowiedzi w przewodnikach po stylach Oracle i Google.

Czy stosowanie wyjątków, takich jak w przykładowym kodzie, jest anty-wzorcem, czy jest to dopuszczalne, biorąc pod uwagę, że alternatywy również nie są bardzo czyste? Jeśli tak, to w jakich okolicznościach byłoby dobrze? I wzajemnie?

Tak, twój kolega ma rację: to zły kod. Jeśli błąd można rozwiązać lokalnie, należy go natychmiast naprawić. Wyjątek nie powinien być zgłaszany, a następnie obsługiwany natychmiast.

Jest to o wiele czystsze niż Twoja wersja ( getValueByKey()metoda została usunięta):

public String getByKey(String key) {
    if (valuesFromDatabase.containsKey(key)) {
        return valuesFromDatabase.get(key);
    } else {
        String defaultValue = defaultValues.get(key);
        valuesFromDatabase.put(key, defaultvalue);
        return defaultValue;
    }
}

Wyjątek należy zgłosić tylko wtedy, gdy nie wiesz, jak rozwiązać błąd lokalnie.

Nie nazwałbym tego zastosowania wyjątków anty-wzorcem, po prostu nie najlepszym rozwiązaniem problemu komunikowania złożonego wyniku.

Najlepszym rozwiązaniem (zakładając, że nadal korzystasz z Java 7) byłoby użycie Opcji Guava; Nie zgadzam się, że jego użycie w tym przypadku byłoby hackerskie. Wydaje mi się, w oparciu o rozszerzone wyjaśnienie Guava o Opcjonalnym , że jest to doskonały przykład tego, kiedy go użyć. Rozróżniasz między „nie znaleziono wartości” a „wartością null znaleziono”.

Ponieważ nie ma żadnych rozważań dotyczących wydajności i jest to szczegół implementacji, ostatecznie nie ma znaczenia, które rozwiązanie wybierzesz. Ale muszę się zgodzić, że to zły styl; nieobecność klucza jest czymś, o czym wiesz, że się wydarzy, a nawet nie radzisz sobie z więcej niż jednym wywołaniem stosu, czyli tam, gdzie wyjątki są najbardziej przydatne.

Podejście krotkowe jest nieco zhackowane, ponieważ drugie pole nie ma znaczenia, gdy wartość logiczna jest fałszywa. Sprawdzanie, czy klucz istnieje wcześniej, jest głupie, ponieważ mapa szuka klucza dwa razy. (Cóż, już to robisz, więc w tym przypadku trzy razy.) OptionalRozwiązanie idealnie pasuje do problemu. Może się wydawać trochę ironiczne, aby przechowywać nullw Optional, ale jeśli to jest to, co użytkownik chce zrobić, nie możesz odmówić.

Jak zauważył Mike w komentarzach, jest z tym problem; ani Guava, ani Java 8 nie Optionalpozwalają na przechowywanie nulls. Dlatego musisz rzucić własny, który - choć prosty - wymaga sporej ilości płyty kotłowej, więc może to być przesada dla czegoś, co zostanie użyte tylko raz wewnętrznie. Możesz także zmienić typ mapy na Map<String, Optional<String>>, ale obsługa Optional<Optional<String>>s staje się niewygodna.

Rozsądnym kompromisem może być utrzymanie wyjątku, ale uznanie jego roli jako „alternatywnego zwrotu”. Utwórz nowy sprawdzony wyjątek z wyłączonym śledzeniem stosu i wrzuć jego wstępnie utworzoną instancję, którą możesz przechowywać w zmiennej statycznej. Jest to tanie - być może tak tanie jak lokalny oddział - i nie można zapomnieć o tym, aby je obsłużyć, więc brak śledzenia stosu nie stanowi problemu.

Jest ta klasa Preferencji, która jest wiadrem dla par klucz-wartość. Wartości zerowe są legalne (to ważne). Oczekujemy, że niektóre wartości mogą jeszcze nie zostać zapisane i chcemy automatycznie obsługiwać te przypadki, inicjując je z predefiniowaną wartością domyślną na żądanie.

Problem jest właśnie taki. Ale już opublikowałeś rozwiązanie:

Odmiana tego podejścia może wykorzystywać Opcjonalną Guawę (Guawa jest już używana w całym projekcie) zamiast jakiejś niestandardowej Tuple, a następnie możemy rozróżnić Optional.absent () od „właściwej” wartości null . Nadal jednak wydaje się hacking z powodów, które są łatwe do zauważenia - wprowadzenie dwóch poziomów „nieważności” wydaje się nadużywać koncepcji stojącej za tworzeniem Opcjonalnych.

Nie używaj jednak null lub Optional . Możesz i powinieneś używać Optionaltylko. Aby poczuć się „hackersko”, po prostu użyj go zagnieżdżonego, aby uzyskać Optional<Optional<String>>wyraźne stwierdzenie, że w bazie danych może być klucz (pierwsza warstwa opcji) i że może on zawierać predefiniowaną wartość (druga warstwa opcji).

Takie podejście jest lepsze niż stosowanie wyjątków i jest dość łatwe do zrozumienia, o ile Optionalnie jest dla Ciebie nowe.

Należy również pamiętać, że Optionalma kilka funkcji komfortu, dzięki czemu nie musisz wykonywać całej pracy sam. Obejmują one:

• static static <T> Optional<T> fromNullable(T nullableReference)przekonwertować dane wejściowe bazy danych na Optionaltypy
• abstract T or(T defaultValue) aby uzyskać wartość klucza wewnętrznej warstwy opcji lub (jeśli nie występuje) uzyskać domyślną wartość klucza

Wiem, że jestem spóźniony na imprezę, ale w każdym razie twój przypadek użycia przypomina, w jaki sposób JavaProperties pozwala również zdefiniować zestaw domyślnych właściwości, które zostaną sprawdzone, jeśli instancja nie załaduje odpowiedniego klucza.

Patrząc, jak odbywa się implementacja Properties.getProperty(String)(z Java 7):

Object oval = super.get(key);
String sval = (oval instanceof String) ? (String)oval : null;
return ((sval == null) && (defaults != null)) ? defaults.getProperty(key) : sval;

Jak już cytowałeś, naprawdę nie ma potrzeby „nadużywać wyjątków w celu kontroli przepływu”.

Podobnym, ale nieco bardziej zwięzłym podejściem do odpowiedzi @ BЈовић może być również:

public String getByKey(String key) {
    if (!valuesFromDatabase.containsKey(key)) {
        valuesFromDatabase.put(key, defaultValues.get(key));
    }
    return valuesFromDatabase.get(key);
}

Chociaż myślę, że odpowiedź @ BЈовић jest dobra na wypadek, gdyby nie getValueByKeybyła potrzebna nigdzie indziej, nie sądzę, aby twoje rozwiązanie było złe, gdyby Twój program zawierał oba przypadki użycia:

• pobieranie według klucza z automatycznym tworzeniem na wypadek, gdyby klucz wcześniej nie istniał, oraz

• pobieranie bez tego automatyzmu, bez zmiany czegokolwiek w bazie danych, repozytorium lub mapie klucza (pomyśl o getValueByKeybyciu publicznym, a nie prywatnym)

Jeśli taka jest Twoja sytuacja i dopóki poprawa wydajności jest akceptowalna, myślę, że proponowane rozwiązanie jest w pełni prawidłowe. Ma tę zaletę, że pozwala uniknąć powielania kodu pobierania, nie opiera się na zewnętrznych strukturach i jest dość prosty (przynajmniej w moich oczach).

W rzeczywistości w takiej sytuacji zależy od kontekstu, czy brakujący klucz jest sytuacją „wyjątkową”, czy nie. W kontekście, w którym się znajduje, getValueByKeypotrzebne jest coś podobnego . W kontekście, w którym oczekiwane jest automatyczne tworzenie klucza, zastosowanie metody, która ponownie wykorzystuje już dostępną funkcję, połyka wyjątek i zapewnia inne zachowanie po awarii, jest całkowicie sensowne. Można to interpretować jako rozszerzenie lub „dekorator” getValueByKey, nie tyle jako funkcję, w której „wyjątek jest nadużywany w celu kontroli”.

Oczywiście istnieje trzecia alternatywa: utwórz trzecią, prywatną metodę zwracającą Tuple<Boolean, String>, jak zasugerowałeś, i ponownie użyj tej metody zarówno w, jak getValueByKeyi getByKey. W przypadku bardziej skomplikowanego przypadku, który może być rzeczywiście lepszą alternatywą, ale w przypadku tak prostego pokazanego tutaj przykładu IMHO ma zapach nadinżynierii i wątpię, aby kod stał się w ten sposób łatwiejszy do utrzymania. Jestem tutaj z najwyższą odpowiedzią tutaj autorstwa Karla Bielefeldta :

„nie powinieneś czuć się źle przy korzystaniu z wyjątków, gdy upraszcza to twój kod”.

Optionaljest poprawnym rozwiązaniem. Jeśli jednak wolisz, istnieje alternatywa, która ma wrażenie „dwóch zer”, rozważ wartownika.

Zdefiniuj prywatny ciąg statyczny keyNotFoundSentinelo wartości new String(""). *

Teraz metoda prywatna może return keyNotFoundSentinelraczej throw new KeyNotFoundException(). Metoda publiczna może sprawdzić tę wartość

String rval = getValueByKey(key);
if (rval == keyNotFoundSentinel) { // reference equality, not string.equals
    ... // generate default value
} else {
    return rval;
}

W rzeczywistości nulljest szczególnym przypadkiem wartownika. Jest to wartosc wartosciowa zdefiniowana przez jezyk, z kilkoma szczególnymi zachowaniami (tj. Dobrze zdefiniowane zachowanie, jesli wywołuje sie metode null). Zdarza się, że taki wskaźnik jest tak przydatny, że używa go prawie każdy język.

* Celowo używamy, new String("")a nie tylko po to, ""aby zapobiec „internowaniu” łańcucha przez Javę, co dałoby mu takie samo odniesienie jak każdy inny pusty ciąg. Ponieważ wykonaliśmy ten dodatkowy krok, mamy gwarancję, że Ciąg, do którego keyNotFoundSentinelodnosi się, jest unikalną instancją, którą musimy upewnić się, że nigdy nie pojawi się na samej mapie.

Ucz się z frameworka, który nauczył się od wszystkich bolących punktów Javy:

.NET oferuje dwa bardziej eleganckie rozwiązania tego problemu, czego przykładem są:

• Dictionary<TKey, TValue>.TryGetValue(TKey, out TValue)
• Nullable<T>.GetValueOrDefault(T default)

Ten drugi jest bardzo łatwy do napisania w Javie, ten pierwszy wymaga jedynie klasy pomocnika „silnego odniesienia”.