Jak obsłużyć błędy bez "catch Exception"?

Kilka lat temu, w poprzedniej firmie pracowałem z człowiekiem, który miał "osobliwe" podejście do czystego kodu, w sensie miał zawsze mnóstwo uwag do czyjegoś kodu, ale do swojego nigdy. W tym projekcie miałem zintegrować istniejącą aplikację z zewnętrznym komponentem do mapowania danych z pewnego sekretnego formatu do XML. Ten komponent dostarczany był jako wygenerowana osobna aplikacja Java jako folder z klasami, który po prostu wrzucałem do src/main/java i można było ten kod wywołać statyczną metodą (pomijam na razie czy to było dobre rozwiązanie - szczeniakiem byłem).

Problem był jednak taki, że ten komponent często się wywalał na błędnych danych wejściowych, nad którymi nie mieliśmy kontroli, a klient bardzo chciał użyć tego rozwiązania. Więc wymyśliłem ficzer, że cały proces mapowania danych (wraz z ewentualnymi błędami) będę zapisywał do pliku i wrzucał go na udział sieciowy, gdzie klient ma dostęp. Ale potrzebowałem mieć loga wszystkich błędów, żeby udowodnić klientowi że błędy są z zewnętrznego komponentu a nie od nas. Więc napisałem następujący kod:

public void doTheMappingJob() {
  try {
    externalComponent.doTheJob(params, toFileLogger);
    toFileLogger.write("Everything went fine");
  catch (Exception e) {
    toFileLogger.write("That mapping component screwed up again", e.getStackTrace());
  }

  toFileLogger.flushToStorage();
}

To działało dokładnie tak, jak chciałem. Ale przy review oczywiście dostałem komentarz

You should not catch Exception.

Próbowałem tłumaczyć mu, że to jest granica nasz kod - czyjś kod, że musimy mieć observability na prodzie nawet kosztem łamania pewnych zasad, ale nie dało się go przekonać. Ostatecznie odpuścił i kod trafił na proda, gdzie działał fajnie. Ale czy moje rozwiązanie i tok rozumowania jest poprawny? Jak spełnilibyście takie wymagania bez catch Exception?

kelog napisał(a):

przy review oczywiście dostałem komentarz

You should not catch Exception.

jak dla mnie to to jest największy WTF i na tym bym zakończył odpisywanie na takie review.

Oczywiście w takim przypadku łapanie Exception ma sens -> są nawet przypadki, gdzie w Javie łapie się Throwable (a to już zupełny hardkor, bo po pewnych błedach JVM nie gwarantuje poprawności dalszego działania i nawet zapis do logu może się nie udać).

Strzelam, że ktoś bezmyślnie aplikuje wyuczoną regułkę.
Jedna rzecz -> ten catch Exception to jest takie miejsce gdzie pewnie dałbym komentarz dlaczego robię coś tak brzydkiego.

Uwaga z review zasadna, łapiesz wyjątek i tylko logujesz, flow przechodzi dalej i ostatecznie "system nie działa jak należy" ;) Rozumiem, że prawdziwy kod wyglądał nieco inaczej?

Podstawowa sprawa, to jak wygląda interfejs do tego komponentu? Czy daje możliwość weryfikacji danych wejściowych? Czy sygnalizuje błędy w inny sposób niż wyjątki?
Czy lecą wyjątki komponentu, czy raczej komponent pcha wyjątki w górę?

Jak ziomkowi od review kod się nie podobał, to dopytywałbym jak chciałby, żeby obsłużyć sytuację, w której zewnętrzny komponent rzuca wyjątkiem np. NotSerializableException albo czymś takim.
Zmuszenie kogoś do pracy koncepcyjnej wygeneruje na 99% odpowiedź "dobra, niech będzie tak jak proponujesz".

You should not catch Exception.

Ta zasada mówi tyle, żeby nie łapać Exceptionów wszędzie tylko tam gdzie ma to sens. Jeśli w twoim wypadku zapisanie do logu nie ma sensu gdzieś wyżej w hierarchi kodu to ok.

Bardziej mnie martwi to, że wywołujecie zewnętrzny komponent i nie oczekujecie na żaden wynik (pozytywny lub negatywny). Przy takim kodzie ciężko powiedzieć jakie jest rozwiązanie optymalne, bo nie wiemy, czy design ma jakikolwiek sens

Result pattern. Ale życzę powodzenia jeśli to już jakiś dojrzały codebase. Niektórym mózgi wybuchną na taki pomysł

rjakubowski napisał(a):

Result pattern. Ale życzę powodzenia jeśli to już jakiś dojrzały codebase. Niektórym mózgi wybuchną na taki pomysł

Czy to jakaś fency nazwa na Either/Try?

A wracając do tematu jeśli w kodzie nie mogę użyć konstrukcji X to trzeba użyć wytrychu czyli dla javy naprzykład Try z vavra XD Try.ofCallable(Callable<? extends T> callable) czy Try.ofSupplier(Supplier<? extends T> supplier) prawdopodobnie rozwiązałby twoje problemy a możliwe iż lider się nie ogarnie co się dzieje :P

BTW sam kiedyś miałem niemiłą sytuację gdy musiałem użyć catch Exception bo jakiś idiota w bibliotece napisał throws Exception :( Już nawet nie pamiętam co to była za biblioteka :D

BTW 2 a catch RuntimeException by nie wystarczyło? Czy już wyłączili w Javie rozróżnienie między Exception i RuntimeException? Kurde, musze wrócić do Javy bo już nie wiem co tam się wydażył :( Ostatnie 3 lata żyłem w szcześliwym świecie gdzie wszystkie wyjątki zachowywały się jak RuntimeException ale chyba ten świat się kończy dla mnie :(

KamilAdam napisał(a):

BTW 2 a catch RuntimeException by nie wystarczyło? Czy już wyłączili w Javie rozróżnienie między Exception i RuntimeException? Kurde, musze wrócić do Javy bo już nie wiem co tam się wydażył :( Ostatnie 3 lata żyłem w szcześliwym świecie gdzie wszystkie wyjątki zachowywały się jak RuntimeException ale chyba ten świat się kończy dla mnie :(

O ile mi wiadomo nadal jest Exception i RuntimeException.
Faktycznie, w teorii, jeśli komponent nie deklaruje throws Exception to wtedy powinno łapać się RuntimeException (i/lub) Throwable (hardkor).
W praktyce mamy sneakyThrow i tym podobne "herezje", powodujące, że w powyższej sytuacji bezpieczniej jest łapać Exception (zamiast RuntimeException), nawet jeśli (throws Exception) nie jest deklarowane.

Dzięki wszystkim za odpowiedzi!

abrakadaber napisał(a):

kelog napisał(a):

przy review oczywiście dostałem komentarz

You should not catch Exception.

jak dla mnie to to jest największy WTF i na tym bym zakończył odpisywanie na takie review.

Mnie się wydaje, że jemu chodziło o to, by nie używać tego generycznego rozwiązania, a nie ogólnie, by nie używać catch. Miałem kilka lat temu takiego nawiedzonego seniora.

Takie rzeczy jak "Do not catch exceptions", to są tylko uproszczenia. Nie są koniecznie błędne, ale też nie zawsze są poprawne. Trzeba znać uzasadnienie za nimi. A uzasadnienie jest takie, że często początkujący programiści widzą błąd javy "Checked exceptions must be caught or rethrown", nie rozumieją co to znaczy więc z automatu obtaczają funkcję try/catchem, mimo że nie powinni - tym samym wyciszając wyjątek, nie obsługując go, i potencjalnie doprowadzając do buga w aplikacji. Często IDE narzeka na pusty catch, więc podpowiada żeby dodać logger, więc wtedy początkujący programiści z automatu dodają pusty logger.

Z tego powodu niektórzy (jak za pewne Twój leader) wyłapali pattern, że "jeśli gdzieś jest catch+logger, to znaczy to że ktoś wyciszył wyjątek bez obsługi". Nauczyli się, że często to jest złe - więc narzekają. Motyw za tym jest więc nawet sensnwony.

Tylko że jak z każdymi absolutami, catch+logger nie zawsze oznacza przemilczany wyjątek. Czasem samo złapanie i zalogowanie wyjątku to właśnie jest obsługa i nie wymaga żadnych dalszych akcji - tak jak w Twoim przypadku. (tylko zaznaczam, że czasami).

Co do samego catch (Exception) - wyjątek, jest to interfejs programistyczny taki sam jak każdy inny. Można informować o błędzie kodem błędu, enumem, messagem, wartością binarną, a można wyjątkiem. Jeśli interfejs modułu który dołączasz komunikuje się wyjątkami, to oczywiście ze integracja z nim będzie polegać na wyjątkach. Jeśli jednak ten wyjątek jest niezamierzony, tzn. biblioteka go rzuca randomowo dla niepoprawnego inputu - to dochodzi pytanie jaki jest faktycznie najlepszy sposób żeby go obsłużyć:

• Jeśli coś się dzieje po Twojej stronie, tzn. wpuszczasz specjalnie złe dane, które mógłbyś zwalidować - to wtedy lepiej go nie łapać
• Jeśli coś się dzieje po "ich strone", tzn. failuje bez powodu, to wtedy faktycznie powinieneś zrobić catch() i go złapać.

A nie proście zapytać co należy robić tego kto napisał czego nie należy robic? Wiemy już że należy should not catch. Ale review powinno powiedzieć co should do.