Code review: zła obsługa wyjątków

Już kiedyś pisałem jak obsługiwać prawidłowe wyjątki ale dzisiaj jeszcze raz chciałbym rozwinąć temat. Zacznijmy od:

private string GetData(int id)
{
    string result=null;
    
    try
    {
        result = _service.GetData(id);
    }
    catch(Exception e)
    {
    }
    return result;
}

Jest to oczywiście skrajnie złe rozwiązanie ponieważ wszystko ignorujemy. Na szczęście programiści rzadko popełniają powyższy błąd. Niestety dużo częściej popełnianym błędem jest:

private string GetData(int id)
{
   string result;
   try
   {
       result = _service.GetData(id);
   }
   catch(Exception e)
   {
       Logger.Log(e.Message);
       // jakas logika
       result = null;
   }
   return result;
}

Pierwszy problemem jest wyłapywanie zbyt ogólnego wyjątku Exception. W zdecydowanej większości przypadków jest to zły nawyk. Klauzuli catch używamy wyłącznie gdy:

  1. Mamy wystarczającą wiedzę co się stało.
  2. Spodziewamy się specyficznego wyjątku.
  3. Potrafimy go naprawić np. poprzez izolację procesu albo w przypadku web service można ponownie spróbować połączyć się z usługą.
  4. Możliwe typy wyjątków zostały rozpoznane już na etapie projektowania.

Co oznacza łapanie wyjątku Exception? Czy możliwe jest, że reakcja na wyjątek podczas wywołania GetData jest zawsze taka sama? Oczywiście jest to niemożliwe – nie jesteśmy w stanie przewidzieć wszystkich wyjątków stąd Exception jest prawie zawsze zły. Co jeśli wywołanie GetData spowodowało OutOfMemoryException? Czy aby na pewno chcemy mieć taką samą obsługę błędów? Z tego względu zawsze łapmy wyjątki, których spodziewaliśmy się na etapie projektowania:

private string GetData(int id)
{
   string result;
   try
   {
       result = _service.GetData(id);
   }
   catch(TimeOutException e)
   {
       // jakas logika odpowiedzialna za recovery mechanism.
   }
   return result;
}

Co z logami? Jeśli chcemy wykonywać je, wtedy lepiej to zrobić w globalnym miejscu a nie w catch’u dla każdej metody.

Analogicznym błędem popełnianym przez bardzo wielu programistów jest wyrzucanie wyjątków typu Exception. Moim zdaniem Exception powinien być typu abstrakcyjnego, ewentualnie zawierać konstruktor niepubliczny. Umożliwienie wyrzucanie wyjątków Exception powoduje, że czasami programiści po prostu muszą łapać ogólne obiekty. ZAWSZE należy wyrzucać wyjątek specyficzny tak, że użytkownicy danej klasy nie musza łapać Exception. Uważam, że nie ma wyjątków od tej reguły.

Dobrym zwyczajem jest tworzenie wyjątków z modyfikatorem sealed i bardzo ostrożne tworzenie klas bazowych dla wyjątków. Wyobraźmy sobie, że programista tworzy następującą klasę:

class TimeoutException:Exception
{
    // kod tutaj  
}

Następny użytkownik napisał kod:

private string GetData(int id)
{
   string result;
   try
   {
       //jaks kod
   }
   catch (TimeoutException e)
   {
       // jakas logika odpowiedzialna za recovery mechanism.
   }
   return result;
}

Co się stanie jeśli autorzy biblioteki stworzą następny wyjątek dziedziczony po TimeoutException?

class AnotherTimeoutException : TimeoutException
{
}

Oczywiście metoda GetData wyłapie wszystkie wyjątki dziedziczące po TimeoutException. Czasami jest to poprawne zachowanie ale należy być świadomym ryzyka tworzenia bazowych klas dla wyjątków. Dobrym zwyczajem jest zatem używanie modyfikatora sealed. Oczywiście są przypadki gdzie powyższe przykłady nie są złe – wszystko zależy od konkretnego scenariusza. Powyższe przykłady miały na celu pokazanie istniejącego ryzyka.

Wspomniałem, że czasami catch(Exception e) jest akceptowalny. Oto przypadki, które kwalifikują się do takiego scenariusza:

  1. Błędna architektura. Miałem już kiedyś przypadek, że ze względu na błąd w zewnętrznej bibliotece to był jedyny sposób na prawidłowe zachowanie aplikacji. Dotyczy to PrintPreview w Windows ale niestety już nie pamiętam dokładnie kodu.
  2. Błędna architektura – jeśli kod jest złe napisany i zawsze wyrzuca typ Exception zamiast specyficznych wyjątków.
  3. W catch jest re-throw, który i tak spowoduje, wyrzucenie wyjątku. Czasami taki mechanizm stosuje się aby dodać specyficzne informacje, zależne od kontekstu. Jeśli na końcu catch jest re-throw wtedy kod jest akceptowalny.

5 thoughts on “Code review: zła obsługa wyjątków”

  1. Nakeży też podkreślić, że istnieją wyjątki, które są teoretycznie zgłaszane lecz nigdy nie powinny się w dobrze napisanej aplikacji pojawić np. ArgumentException czy ArgumentNullException. Skoro kontrakt mówi, że się pojawią w pewnych konkretnych okolicznościach to ich zgłoszenie oznacza nasz błąd, który musimy usunąć a nie “uciszyć” przez try/catch.

  2. @Paweł:
    Moim zdaniem masz niebezpieczne podejście do Try-Catch. To nie służy nigdy aby uciszyć wyjątek. Jeśli tak jest to znaczy ze zle to zostało zaprojektowane.

  3. Kolejnym błędem jest taka konstrukcja:
    try {
    // some code
    } catch(Exception ex){
    // jakaś logika
    throw ex;
    }
    Powoduje to utrate części informacjimo wyjątku (o ile dobrze pamiętam nadpisywany jest StackTrace ale ręki sobie za to uciąć nie dam)

  4. Do obszarów w których będziemy przechwytywać wyjątki wszelkiego rodzaju dodał bym sytuację gdzie odpalamy delegowany kod, gdzie nie wiemy co nam życie przyniesie i w ramach takiej logicznej transakcji pilnujemy by “user” nam ekosystemu nie posypał.

Leave a Reply

Your email address will not be published.