Czy powinienem rejestrować błędy konstruktorów zgłaszających wyjątki?

Tworzyłem aplikację przez kilka miesięcy i zdałem sobie sprawę, że pojawił się wzorzec:

logger.error(ERROR_MSG);
throw new Exception(ERROR_MSG);

Lub podczas łapania:

try { 
    // ...block that can throw something
} catch (Exception e) {
    logger.error(ERROR_MSG, e);
    throw new MyException(ERROR_MSG, e);
}

Tak więc, ilekroć rzucałem lub łapałem wyjątek, rejestrowałem go. W rzeczywistości było to prawie całe logowanie, które zrobiłem w aplikacji (oprócz czegoś do inicjalizacji aplikacji).

Jako programista unikam powtórzeń; Postanowiłem więc przenieść wywołania programu rejestrującego do konstrukcji wyjątku, więc za każdym razem, gdy budowałem wyjątek, rzeczy były rejestrowane. Mógłbym również z pewnością stworzyć ExceptionHelper, który wyrzucił dla mnie wyjątek, ale to utrudniłoby interpretację mojego kodu, a co gorsza, kompilator nie poradziłby sobie z nim dobrze, nie zdając sobie sprawy, że wywołanie tego członka rzuć natychmiast.

Czy to jest anty-wzór? Jeśli tak, to dlaczego?

Nie jestem pewien, czy kwalifikuje się jako anty-wzorzec, ale IMO to zły pomysł: niepotrzebne łączenie w celu przeplatania wyjątku z logowaniem.

Nie zawsze możesz chcieć rejestrować wszystkie wystąpienia danego wyjątku (być może ma to miejsce podczas sprawdzania poprawności danych wejściowych, których rejestrowanie może być zarówno obszerne, jak i nieciekawe).

Po drugie, możesz zdecydować się na rejestrowanie różnych wystąpień błędu z różnymi poziomami rejestrowania, w którym to momencie musisz określić to podczas konstruowania wyjątku, co ponownie oznacza mętnienie tworzenia wyjątku przez zachowanie rejestrowania.

Wreszcie, co jeśli wyjątki wystąpiły podczas rejestrowania innego wyjątku? Zalogowałbyś to? Robi się bałagan ...

Twoje wybory to:

• Złap, zaloguj się i (ponownie) rzucaj, jak podałeś w swoim przykładzie
• Utwórz klasę ExceptionHelper, aby zrobić jedno i drugie dla ciebie, ale pomocnicy wyczuwają zapach kodu i nie poleciłbym tego też.
• Przenieś obsługę wyjątków typu catch-all na wyższy poziom
• Rozważ AOP jako bardziej wyrafinowane rozwiązanie problemów przekrojowych, takich jak rejestrowanie i obsługa wyjątków (ale o wiele bardziej skomplikowane niż tylko posiadanie tych dwóch linii w blokach catch;))

Jako programista unikam powtórzeń [...]

Istnieje niebezpieczeństwo, gdy koncepcja "don't repeat yourself"traktuje się zbyt poważnie do tego stopnia, że ​​staje się zapachem.

Do echa @ Dan1701

Chodzi o rozdzielenie obaw - przeniesienie logowania do wyjątku powoduje ścisłe powiązanie wyjątku z rejestrowaniem, a także oznacza, że ​​dodałeś dodatkową odpowiedzialność do wyjątku rejestrowania, co z kolei może stworzyć zależności dla klasy wyjątku, że on nie potrzebuje.

Z punktu widzenia konserwacji możesz (chciałbym) argumentować, że ukrywasz fakt, że wyjątek jest rejestrowany przed opiekunem (przynajmniej w kontekście czegoś takiego jak przykład), zmieniasz także kontekst ( od położenia procedury obsługi wyjątku do konstruktora wyjątku), co prawdopodobnie nie jest tym, czego zamierzałeś.

Wreszcie zakładasz, że zawsze chcesz zapisać wyjątek, dokładnie w ten sam sposób, w momencie, w którym został utworzony / podniesiony - co prawdopodobnie nie jest prawdą. Chyba że będziesz mieć wyjątki dotyczące rejestrowania i braku rejestrowania, które szybko stałyby się bardzo nieprzyjemne.

Więc w tym przypadku myślę, że „SRP” przebija „SUCHY”.

Twój błąd rejestruje wyjątek, w którym nie możesz go obsłużyć, a zatem nie możesz wiedzieć, czy rejestrowanie jest częścią prawidłowej obsługi.
Istnieje bardzo niewiele przypadków, w których można lub trzeba poradzić sobie z częścią błędu, która może obejmować rejestrowanie, ale nadal musi sygnalizować błąd dzwoniącemu. Przykładami są nieczytelne błędy odczytu i tym podobne, jeśli wykonujesz odczyt najniższego poziomu, ale ogólnie mają one wspólną cechę, że informacje przekazywane do osoby dzwoniącej są poważnie filtrowane, ze względu na bezpieczeństwo i użyteczność.

Jedyną rzeczą, którą możesz zrobić w twoim przypadku i z jakiegoś powodu musisz zrobić, jest przetłumaczenie wyjątku, który implementuje zgłasza, na taki, jakiego oczekuje osoba wywołująca, połączenie łańcucha oryginału z kontekstem i pozostawienie wszystkiego innego w spokoju.

Podsumowując, twój kod przypisał prawo i obowiązek częściowej obsługi wyjątku, naruszając w ten sposób SRP.
DRY nie wchodzi w to.

Ponowne sprawdzenie wyjątku tylko dlatego, że zdecydowałeś się go zarejestrować za pomocą bloku catch (co oznacza, że ​​wyjątek wcale się nie zmienił) jest złym pomysłem.

Jednym z powodów, dla których używamy wyjątków, komunikatów o wyjątkach i ich obsługi jest to, abyśmy wiedzieli, co poszło nie tak i sprytnie napisane wyjątki mogą znacznie przyspieszyć znalezienie błędu.

Pamiętaj też, że obsługa wyjątków kosztuje znacznie więcej zasobów niż, powiedzmy, mieć if, więc nie powinieneś zajmować się nimi zbyt często tylko dlatego, że masz na to ochotę. Ma to wpływ na wydajność Twojej aplikacji.

Jednak dobrym rozwiązaniem jest użycie wyjątku jako środka do zaznaczenia warstwy aplikacji, w której wystąpił błąd.

Rozważ następujący półpseudo kod:

interface ICache<T, U>
{
    T GetValueByKey(U key); // may throw an CacheException
}

class FileCache<T, U> : ICache<T, U>
{
    T GetValueByKey(U key)
    {
        throw new CacheException("Could not retrieve object from FileCache::getvalueByKey. The File could not be opened. Key: " + key);
    }
}

class RedisCache<T, U> : ICache<T, U>
{
    T GetValueByKey(U key)
    {
        throw new CacheException("Could not retrieve object from RedisCache::getvalueByKey. Failed connecting to Redis server. Redis server timed out. Key: " + key);
    }
}

class CacheableInt
{
    ICache<int, int> cache;
    ILogger logger;

    public CacheableInt(ICache<int, int> cache, ILogger logger)
    {
        this.cache = cache;
        this.logger = logger;
    }

    public int GetNumber(int key) // may throw service exception
    {
        int result;

        try {
            result = this.cache.GetValueByKey(key);
        } catch (Exception e) {
            this.logger.Error(e);
            throw new ServiceException("CacheableInt::GetNumber failed, because the cache layer could not respond to request. Key: " + key);
        }

        return result;
    }
}

class CacheableIntService
{
    CacheableInt cacheableInt;
    ILogger logger;

    CacheableInt(CacheableInt cacheableInt, ILogger logger)
    {
        this.cacheableInt = cacheableInt;
        this.logger = logger;
    }

    int GetNumberAndReturnCode(int key)
    {
        int number;

        try {
            number = this.cacheableInt.GetNumber(key);
        } catch (Exception e) {
            this.logger.Error(e);
            return 500; // error code
        }

        return 200; // ok code
    }
}

Załóżmy, że ktoś zadzwonił GetNumberAndReturnCodei odebrał 500kod, sygnalizując błąd. Zadzwoni do pomocy technicznej, która otworzy plik dziennika i zobaczy to:

ERROR: 12:23:27 - Could not retrieve object from RedisCache::getvalueByKey. Failed connecting to Redis server. Redis server timed out. Key: 28
ERROR: 12:23:27 - CacheableInt::GetNumber failed, because the cache layer could not respond to request. Key: 28

Następnie programista natychmiast wie, która warstwa oprogramowania spowodowała przerwanie procesu, i ma łatwy sposób na zidentyfikowanie problemu. W tym przypadku jest to krytyczne, ponieważ przekroczenie limitu czasu Redis nigdy nie powinno się zdarzyć.

Być może inny użytkownik wywołałby tę samą metodę, również otrzymał 500kod, ale dziennik pokazałby:

INFO: 11:11:11- Could not retrieve object from RedisCache::getvalueByKey. Value does not exist for the key 28.
INFO: 11:11:11- CacheableInt::GetNumber failed, because the cache layer could not find any data for the key 28.

W takim przypadku obsługa może po prostu odpowiedzieć użytkownikowi, że żądanie jest nieprawidłowe, ponieważ żąda wartości dla nieistniejącego identyfikatora.

streszczenie

Jeśli zajmujesz się wyjątkami, upewnij się, że obchodzisz się z nimi we właściwy sposób. Upewnij się także, że Twoje wyjątki obejmują przede wszystkim prawidłowe dane / komunikaty, zgodnie z warstwami architektury, aby komunikaty pomogły Ci zidentyfikować problem, który może wystąpić.

Myślę, że problem jest na bardziej podstawowym poziomie: rejestrujesz błąd i zgłaszasz go jako wyjątek w tym samym miejscu. To jest anty-wzór. Oznacza to, że ten sam błąd jest rejestrowany wiele razy, jeśli zostanie złapany, być może zawinięty w inny wyjątek i ponownie zgłoszony.

Zamiast tego sugeruję rejestrowanie błędów nie po utworzeniu wyjątku, ale po jego przechwyceniu. (Do tego, oczywiście, należy upewnić się, że zawsze jest złowionych gdzieś.) Gdy wyjątek zostanie złapany, to bym tylko zalogować jego StackTrace jeśli jest nie ponownie rzucony lub owinięte jako przyczyna do innego wyjątku. W każdym razie ślady stosu i komunikaty zapakowanych wyjątków są rejestrowane w stosach jako „Spowodowane przez ...”. Łapacz może również zdecydować np. O ponownej próbie bez zarejestrowania błędu przy pierwszej awarii, lub po prostu potraktować to jako ostrzeżenie lub cokolwiek innego.

Wiem, że to stary wątek, ale właśnie natknąłem się na podobny problem i wymyśliłem podobne rozwiązanie, więc dodam moje 2 centy.

Nie kupuję argumentu dotyczącego naruszenia SRP. W każdym razie nie do końca. Załóżmy, że 2 rzeczy: 1. Naprawdę chcesz rejestrować wyjątki w momencie ich wystąpienia (na poziomie śledzenia, aby móc odtworzyć przebieg programu). Nie ma to nic wspólnego z obsługą wyjątków. 2. Nie możesz lub nie będziesz używać do tego AOP - zgadzam się, że byłby to najlepszy sposób, ale niestety utknąłem w języku, który nie zapewnia do tego narzędzi.

Z mojego punktu widzenia jesteś w zasadzie skazany za naruszenie SRP na dużą skalę, ponieważ każda klasa, która chce zgłaszać wyjątki, musi znać dziennik. Przeniesienie rejestrowania do klasy wyjątków w rzeczywistości znacznie zmniejsza naruszenie SRP, ponieważ teraz tylko wyjątek narusza to, a nie wszystkie klasy w bazie kodu.

To jest anty-wzór.

Moim zdaniem, wywołanie rejestrowania w konstruktorze wyjątku byłoby przykładem: Flaw: Constructor robi Real Work .

Nigdy nie oczekiwałbym (ani nie chciałbym) od konstruktora, aby wykonał jakieś zewnętrzne wezwanie do serwisu. To bardzo niepożądany efekt uboczny, który, jak zauważa Miško Hevery, zmusza podklasy i kpiny do dziedziczenia niepożądanych zachowań.

Jako takie, naruszyłoby to również zasadę najmniejszego zdziwienia .

Jeśli tworzysz aplikację z innymi, to ten efekt uboczny prawdopodobnie nie będzie dla nich widoczny. Nawet jeśli pracujesz sam, możesz o tym zapomnieć i zaskoczyć się na drodze.